Plumb render view ID from audio-related code in renderer through IPCs to AudioRendererHost in brows…

Plumb render view ID from audio-related code in renderer through IPCs to AudioRendererHost in browser process.  This is a first step towards implementing three major Chromium features, all of which need to associate audio outputs with their source tabs.  See BUGs referenced by this change for more details.

There are a two places where refactoring work is still needed to be able to plumb in render_view_ids.  As the refactoring work is well out-of-scope for this change, stubs with TODO comments were added as placemarkers.  Nevertheless, this change has been confirmed to function for the majority of use cases on the web.

Testing of render_view_id plumbing by audio output method:

  <audio>      works
  Pepper       works
  WebAudio     stub works
  WebRTC       stub works
  NPAPI/Flash  not tested (N/A)

BUG=3541, 64215, 153392
TEST=Ran all media_unittests and content_unittests to confirm no breakage.  Also, ran a debug build and manually visited websites using the above list of methods to confirm that no audio recording/playback regressions were introduced.

[miu, 2012-10-16 05:00:51]

Reviewers,

This is a "take-over" of Issue 10836025
(http://chromiumcodereview.appspot.com/10836025).  Note that you've already
LGTM'ed this work (in 10836025).  Hopefully, all you need to do is examine the
differences, which are:

1. No WebKit change needs to land for this!  :-)
2. All outstanding reviewer comments have been addressed.
3. Instead of adding content/renderer/media/audio_output_ipc_impl.*, the class
has been made a simple internal class of AudioMessageFilter.
4. Due to WebRTC changes over the last few months, it was not possible to plumb
in render_view_id there.  Some significant refactoring is needed first.
5. Bug fix: AudioRendererMixerManager maintains separate mixers for separate
render views.

[miu, 2012-10-16 05:11:33]

+cc sail@

[tommi (sloooow) - chröme, 2012-10-16 09:17:06]

https://codereview.chromium.org/11166002/diff/1/content/renderer/media/audio_...
File content/renderer/media/audio_device_factory.cc (right):

https://codereview.chromium.org/11166002/diff/1/content/renderer/media/audio_...
content/renderer/media/audio_device_factory.cc:22: int render_view_id) {
Why don't we do the same for the input device?

We've historically had a problem with the output and input implementations
diverging, so if we need to do this for both, let's do it now.

[miu, 2012-10-16 23:43:49]

https://codereview.chromium.org/11166002/diff/1/content/renderer/media/audio_...
File content/renderer/media/audio_device_factory.cc (right):

https://codereview.chromium.org/11166002/diff/1/content/renderer/media/audio_...
content/renderer/media/audio_device_factory.cc:22: int render_view_id) {
On 2012/10/16 09:17:07, tommi wrote:
> Why don't we do the same for the input device?
> 
> We've historically had a problem with the output and input implementations
> diverging, so if we need to do this for both, let's do it now.

Done.

[tommi (sloooow) - chröme, 2012-10-17 18:40:06]

lgtm! (with a few nits).
thanks for taking care of the input device.

https://codereview.chromium.org/11166002/diff/5001/content/renderer/media/aud...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/11166002/diff/5001/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:56: private:
indent

https://codereview.chromium.org/11166002/diff/5001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.cc (right):

https://codereview.chromium.org/11166002/diff/5001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:38: if (ipc_.get())
{
nit: I don't think we need to call .get() like this anymore for scoped_ptr (see
chromium-dev), so you can revert the added .get()'s in this file.

https://codereview.chromium.org/11166002/diff/5001/media/audio/audio_input_de...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/11166002/diff/5001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:53: CHECK(ipc_.get());
.get() isn't necessary

https://codereview.chromium.org/11166002/diff/5001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:257: if (ipc_.get()) {
ditto

https://codereview.chromium.org/11166002/diff/5001/media/audio/audio_output_d...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/11166002/diff/5001/media/audio/audio_output_d...
media/audio/audio_output_device.cc:54: CHECK(ipc_.get());
.get()

[miu, 2012-10-17 20:10:43]

Good catch on the scoped_ptr change.  The chromium-dev mailing list is a bit
spammy, so I missed that.

https://codereview.chromium.org/11166002/diff/5001/content/renderer/media/aud...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/11166002/diff/5001/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:56: private:
On 2012/10/17 18:40:06, tommi wrote:
> indent

Done.

https://codereview.chromium.org/11166002/diff/5001/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.cc (right):

https://codereview.chromium.org/11166002/diff/5001/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:38: if (ipc_.get())
{
On 2012/10/17 18:40:06, tommi wrote:
> nit: I don't think we need to call .get() like this anymore for scoped_ptr
(see
> chromium-dev), so you can revert the added .get()'s in this file.

Done.

https://codereview.chromium.org/11166002/diff/5001/media/audio/audio_input_de...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/11166002/diff/5001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:53: CHECK(ipc_.get());
On 2012/10/17 18:40:06, tommi wrote:
> .get() isn't necessary

Done.

https://codereview.chromium.org/11166002/diff/5001/media/audio/audio_input_de...
media/audio/audio_input_device.cc:257: if (ipc_.get()) {
On 2012/10/17 18:40:06, tommi wrote:
> ditto

Done.

https://codereview.chromium.org/11166002/diff/5001/media/audio/audio_output_d...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/11166002/diff/5001/media/audio/audio_output_d...
media/audio/audio_output_device.cc:54: CHECK(ipc_.get());
On 2012/10/17 18:40:06, tommi wrote:
> .get()

Done.

[Chris Rogers, 2012-10-17 20:43:10]

+dalecurtis to double-check

How much has this been tested for all the different audio clients: WebRTC,
WebAudio, <audio>, Pepper, Flash?

Please add testing information in change log

https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_renderer_host.cc:200: VLOG(1) <<
"AudioInputRendererHost@" << this
shouldn't this be a DVLOG?

https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:211: VLOG(1) <<
"AudioRendererHost@" << this
DVLOG

[miu, 2012-10-17 22:06:30]

Addressed Chris' comments (updated issue text with testing info).

https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
content/browser/renderer_host/media/audio_input_renderer_host.cc:200: VLOG(1) <<
"AudioInputRendererHost@" << this
On 2012/10/17 20:43:11, Chris Rogers wrote:
> shouldn't this be a DVLOG?

Done.

https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
content/browser/renderer_host/media/audio_renderer_host.cc:211: VLOG(1) <<
"AudioRendererHost@" << this
On 2012/10/17 20:43:11, Chris Rogers wrote:
> DVLOG

Done.

[Chris Rogers, 2012-10-17 22:43:01]

By testing, I meant to make sure that all the audio clients still work, play
sound correctly, and have no regressions.  From your updated description, it
sounds like WebAudio, etc. are now broken.  I think what you mean is that the
viewID is simply not plumbed through WebAudio yet.  So it'd be good to update
the description to clarify that, and include a separate TEST= section (below
BUG=) describing your tests to verify that all the audio clients still play
audio correctly.



On 2012/10/17 22:06:30, Yuri wrote:
> Addressed Chris' comments (updated issue text with testing info).
> 
>
https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
> File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):
> 
>
https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/media/audio_input_renderer_host.cc:200: VLOG(1)
<<
> "AudioInputRendererHost@" << this
> On 2012/10/17 20:43:11, Chris Rogers wrote:
> > shouldn't this be a DVLOG?
> 
> Done.
> 
>
https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
> File content/browser/renderer_host/media/audio_renderer_host.cc (right):
> 
>
https://codereview.chromium.org/11166002/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/media/audio_renderer_host.cc:211: VLOG(1) <<
> "AudioRendererHost@" << this
> On 2012/10/17 20:43:11, Chris Rogers wrote:
> > DVLOG
> 
> Done.

[scherkus (not reviewing), 2012-10-18 01:56:28]

cool!

I've got a suggestion for enforcing ownership transfer via scoped_ptr<T> instead
of relying on T* and comments that people may not read. Don't hesitate to IM me
if you try it out and it ends up being a massive yak shave, in which case we can
defer to a follow up CL.

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
File content/renderer/media/audio_device_factory.h (right):

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
content/renderer/media/audio_device_factory.h:27: static
media::AudioRendererSink* NewOutputDevice(
nit: this appears to fit on one line

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
File content/renderer/media/audio_input_message_filter.h (right):

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
content/renderer/media/audio_input_message_filter.h:29: media::AudioInputIPC*
CreateAudioInputIPC(int render_view_id);
instead of a comment that people won't read, how about we enforce ownership
transfer be returning scoped_ptr<media::AudioInputIPC>?

inside the .cc the function can allocate to a scoped_ptr<> object and return it
by calling .Pass()

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
File content/renderer/media/audio_message_filter.h (right):

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
content/renderer/media/audio_message_filter.h:30: media::AudioOutputIPC*
CreateAudioOutputIPC(int render_view_id);
ditto for scoped_ptr<>

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:202: // separate
WebRtcAudioDevice per render view.  For now, here's a stub:
as crogers@ mentions, filing + linking to a crbug.com would be good here

https://codereview.chromium.org/11166002/diff/17001/media/audio/audio_output_...
File media/audio/audio_output_device.h (right):

https://codereview.chromium.org/11166002/diff/17001/media/audio/audio_output_...
media/audio/audio_output_device.h:102: AudioOutputDevice(AudioOutputIPC* ipc,
ditto for accepting a scoped_ptr<>

[miu, 2012-10-18 03:27:26]

Chris/Andrew,

Addressed all your comments, and created http://crbug.com/156535 to track
remaining work.

Thanks,
Yuri

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
File content/renderer/media/audio_device_factory.h (right):

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
content/renderer/media/audio_device_factory.h:27: static
media::AudioRendererSink* NewOutputDevice(
On 2012/10/18 01:56:28, scherkus wrote:
> nit: this appears to fit on one line

Done.

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
File content/renderer/media/audio_input_message_filter.h (right):

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
content/renderer/media/audio_input_message_filter.h:29: media::AudioInputIPC*
CreateAudioInputIPC(int render_view_id);
On 2012/10/18 01:56:28, scherkus wrote:
> instead of a comment that people won't read, how about we enforce ownership
> transfer be returning scoped_ptr<media::AudioInputIPC>?
> 
> inside the .cc the function can allocate to a scoped_ptr<> object and return
it
> by calling .Pass()

Done.  I like this enforcement of the contract via code.

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
File content/renderer/media/audio_message_filter.h (right):

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
content/renderer/media/audio_message_filter.h:30: media::AudioOutputIPC*
CreateAudioOutputIPC(int render_view_id);
On 2012/10/18 01:56:28, scherkus wrote:
> ditto for scoped_ptr<>

Done.

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:202: // separate
WebRtcAudioDevice per render view.  For now, here's a stub:
On 2012/10/18 01:56:28, scherkus wrote:
> as crogers@ mentions, filing + linking to a http://crbug.com would be good
here

Done.  http://crbug.com/156535

https://codereview.chromium.org/11166002/diff/17001/media/audio/audio_output_...
File media/audio/audio_output_device.h (right):

https://codereview.chromium.org/11166002/diff/17001/media/audio/audio_output_...
media/audio/audio_output_device.h:102: AudioOutputDevice(AudioOutputIPC* ipc,
On 2012/10/18 01:56:28, scherkus wrote:
> ditto for accepting a scoped_ptr<>

Done (and audio_input_device.h too).

[scherkus (not reviewing), 2012-10-18 05:17:34]

lgtm w/ indent nits

thanks miu@!

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
File content/renderer/media/audio_input_message_filter.h (right):

https://codereview.chromium.org/11166002/diff/17001/content/renderer/media/au...
content/renderer/media/audio_input_message_filter.h:29: media::AudioInputIPC*
CreateAudioInputIPC(int render_view_id);
On 2012/10/18 03:27:26, Yuri wrote:
> On 2012/10/18 01:56:28, scherkus wrote:
> > instead of a comment that people won't read, how about we enforce ownership
> > transfer be returning scoped_ptr<media::AudioInputIPC>?
> > 
> > inside the .cc the function can allocate to a scoped_ptr<> object and return
> it
> > by calling .Pass()
> 
> Done.  I like this enforcement of the contract via code.

Me too :)

https://codereview.chromium.org/11166002/diff/9005/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

https://codereview.chromium.org/11166002/diff/9005/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:145:
CreateAudioInputIPC(render_view_id);
fix indent

https://codereview.chromium.org/11166002/diff/9005/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.cc (right):

https://codereview.chromium.org/11166002/diff/9005/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:113:
CreateAudioOutputIPC(render_view_id);
fix indent

[miu, 2012-10-18 05:28:18]

Chris/Dale,

LGTY too?

https://codereview.chromium.org/11166002/diff/9005/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_platform_audio_input_impl.cc (right):

https://codereview.chromium.org/11166002/diff/9005/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_audio_input_impl.cc:145:
CreateAudioInputIPC(render_view_id);
On 2012/10/18 05:17:35, scherkus wrote:
> fix indent

Done.

https://codereview.chromium.org/11166002/diff/9005/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_platform_audio_output_impl.cc (right):

https://codereview.chromium.org/11166002/diff/9005/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_audio_output_impl.cc:113:
CreateAudioOutputIPC(render_view_id);
On 2012/10/18 05:17:35, scherkus wrote:
> fix indent

Done.

[DaleCurtis, 2012-10-18 06:39:30]

lgtm

[commit-bot: I haz the power, 2012-10-18 06:52:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/11166002/7014

[commit-bot: I haz the power, 2012-10-18 06:52:14]

Failed to apply patch for
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/media/audio_renderer_mixer_manager_unittest.cc
  Hunk #1 FAILED at 18.
  1 out of 4 hunks FAILED -- saving rejects to file
content/renderer/media/audio_renderer_mixer_manager_unittest.cc.rej

Patch:       content/renderer/media/audio_renderer_mixer_manager_unittest.cc
Index: content/renderer/media/audio_renderer_mixer_manager_unittest.cc
diff --git a/content/renderer/media/audio_renderer_mixer_manager_unittest.cc
b/content/renderer/media/audio_renderer_mixer_manager_unittest.cc
index
818bed8b91d04f2483243d7179e7b9ad2cb06daa..52899c8328478025531ca774375d0c928b0c9676
100644
--- a/content/renderer/media/audio_renderer_mixer_manager_unittest.cc
+++ b/content/renderer/media/audio_renderer_mixer_manager_unittest.cc
@@ -18,6 +18,7 @@ static const int kBitsPerChannel = 16;
 static const int kSampleRate = 48000;
 static const int kBufferSize = 8192;
 static const ChannelLayout kChannelLayout = CHANNEL_LAYOUT_STEREO;
+static const int kRenderViewId = -2;
 
 // By sub-classing AudioDeviceFactory we've overridden the factory to use our
 // CreateAudioDevice() method globally.
@@ -27,14 +28,16 @@ class MockAudioRenderSinkFactory : public AudioDeviceFactory
{
   virtual ~MockAudioRenderSinkFactory() {}
 
  protected:
-  virtual media::MockAudioRendererSink* CreateOutputDevice() OVERRIDE {
+  virtual media::MockAudioRendererSink* CreateOutputDevice(int render_view_id)
+      OVERRIDE {
     media::MockAudioRendererSink* sink = new media::MockAudioRendererSink();
     EXPECT_CALL(*sink, Start());
     EXPECT_CALL(*sink, Stop());
     return sink;
   }
 
-  virtual media::AudioInputDevice* CreateInputDevice() OVERRIDE {
+  virtual media::AudioInputDevice* CreateInputDevice(int render_view_id)
+      OVERRIDE {
     ADD_FAILURE();
     return NULL;
   }
@@ -52,16 +55,22 @@ class AudioRendererMixerManagerTest : public testing::Test {
   }
 
   media::AudioRendererMixer* GetMixer(const media::AudioParameters& params) {
-    return manager_->GetMixer(params);
+    return manager_->GetMixer(kRenderViewId, params);
   }
 
   void RemoveMixer(const media::AudioParameters& params) {
-    return manager_->RemoveMixer(params);
+    return manager_->RemoveMixer(kRenderViewId, params);
   }
 
   // Number of instantiated mixers.
   int mixer_count() {
-    return manager_->mixers_.size();
+    int count = 0;
+    typedef AudioRendererMixerManager::AudioRendererMixerMap MixerMap;
+    for (MixerMap::const_iterator it = manager_->mixers_.begin();
+         it != manager_->mixers_.end(); ++it) {
+      count += it->second.size();
+    }
+    return count;
   }
 
  protected:
@@ -119,7 +128,8 @@ TEST_F(AudioRendererMixerManagerTest, CreateInput) {
 
   // Create a mixer input and ensure it doesn't instantiate a mixer yet.
   EXPECT_EQ(mixer_count(), 0);
-  scoped_refptr<media::AudioRendererMixerInput> input(manager_->CreateInput());
+  scoped_refptr<media::AudioRendererMixerInput> input(
+      manager_->CreateInput(kRenderViewId));
   EXPECT_EQ(mixer_count(), 0);
 
   // Implicitly test that AudioRendererMixerInput was provided with the
expected

[commit-bot: I haz the power, 2012-10-18 18:48:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/11166002/16045

[commit-bot: I haz the power, 2012-10-18 18:49:22]

Presubmit check for 11166002-16045 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/renderer

Presubmit checks took 6.5s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[miu, 2012-10-18 18:51:31]

jamesr@,

I need your LGTM for owners approval for files in content/renderer.

Thanks,
Yuri

[jamesr, 2012-10-19 19:30:30]

It's a little unclear to me how this is supposed to work.  What render view ID
is supposed to be associated with a given audio source?  Consider using the
WebAudio API in a chat popup spawned from gmail.  You have to RenderViewImpls
with different IDs (the main tab and the popup).  Gmail's JS is controlling
both, running script in both contexts.  Contexts from either the main page or
the popup may construct new AudioContexts and pass references to them back and
forth.  Which render view ID is associated with the audio source?  What happens
if after the audio source is created gmail opens a new popup window and passes a
reference to the audio context into this new context?  The same situation exists
between tabs that have script links to each other, so it's not just popup
windows.

I ask because the canvas code used to make assumptions about associations
between tabs and HTMLCanvasElements that turned out to not be true in practice. 
Pages were driving multiple popups/tabs from one JS source and passing DOM
elements and JS references across different tabs.  It turns out there wasn't a
good answer for knowing which RenderViewImpl "owns" a particular canvas, a given
canvas could be used in multiple.  For canvas we ended up associating canvases
with a renderer process (which is a group of RenderViews), which was enough for
our purposes.  It might not be for yours, depending on the use cases.

I'm not familiar enough with WebRTC to know if the same issues apply, but if it
behaves the way that most other Web APIs behave I expect it is.

One idea that Darin has mentioned in the past is creating a chromium concept of
a ViewGroup (or WidgetGroup) that would be the set of views/widgets that can
script each other in JS.  Perhaps that is the concept you want?

[jamesr, 2012-10-19 19:30:50]

Just to be clear, I'm pretty sure this patch as is will not do what you want.

[miu, 2012-10-19 20:59:13]

Short story: I do believe this change is doing exactly what I (and the others)
want/need at this time.  I also agree with you that there's a lot more work to
do after this, as this is not a final solution.  ;-)

I'll admit I had not considered your perspective on this change (w.r.t. apps and
popups).  I definitely want to make sure these areas are properly addressed. 
However, I think it makes sense to postpone a solution at this time as it's not
directly relevant to what I'm focusing on here.

Let me provide the wider context for this change.  My end-goal is to provide
tab-level audio/video screencasting within Chrome.  To be more specific,
extension code will be able to call a getTabMedia() function (analogous to
getUserMedia()) to create a WebRTC MediaStream.  The MediaStream will then send
encoded data over a network to a receiver which will play the stream back like a
video broadcast.  Details:
http://www.chromium.org/developers/design-documents/extensions/proposed-chang...

At the moment, we're working closely with the WebRTC and other Chrome media
teams to gradually piece this functionality together over a series of changes to
Chrome.  We're all aware that there are some design and code organization issues
that need to be worked out, and that it will take several changes over the
coming months to provide a completely working implementation.

What seems to work for the vast majority of cases is to consider the creator of
the audio stream as the source of it as well.  So, it's a pretty decent solution
for now.  That said, I have some ideas about how to more-accurately identify the
real source.  I will hash this out with the media teams.

In the meantime, can I proceed with my change here?  It doesn't break any
existing functionality, or anything like that.

Thanks,
Yuri

[stevenjb, 2012-10-25 17:07:37]

I'm not at all familiar with the audio code, but when working on
http://codereview.chromium.org/11236052/, I needed both the render view id and
the render process id to associate a media stream with a tab. I don't see any
references to the render process id here, is that already available?

Also, again I am unfamiliar with the code, but it seems like we are adding an
awful lot of extra parameter passing here. Is there maybe some other way to
retrieve the associated render process / view id when it is needed?

[miu1, 2012-10-25 18:06:16]

stevenjb@,

There's a lot of history prior to this change, as this is a "take-over" of
Issue 10836025 (http://chromiumcodereview.appspot.com/10836025).   The
"lots of extra parameter passing" was discussed there, and the end decision
was to go with it for now.

As for needing render_process_id, there is one AudioRendererHost instance
created and owned by each RenderProcessHost, so it will be trivial to plumb
that in when it's needed for later changes (coming soon).

Thanks for taking a look.  I'll keep you in the loop with the upcoming
plumbing work.

-Yuri



On Thu, Oct 25, 2012 at 10:07 AM, <stevenjb@chromium.org> wrote:

> I'm not at all familiar with the audio code, but when working on
>
http://codereview.chromium.**org/11236052/<http://codereview.chromium.org/112...,
> I needed both the render view id and
> the render process id to associate a media stream with a tab. I don't see
> any
> references to the render process id here, is that already available?
>
> Also, again I am unfamiliar with the code, but it seems like we are adding
> an
> awful lot of extra parameter passing here. Is there maybe some other way to
> retrieve the associated render process / view id when it is needed?
>
>
>
https://codereview.chromium.**org/11166002/<https://codereview.chromium.org/1...
>

[stevenjb (google-dont-use), 2012-10-25 18:09:14]

OK, sounds good, thanks.


On Thu, Oct 25, 2012 at 11:06 AM, Yuri Wiitala <miu@google.com> wrote:

> stevenjb@,
>
> There's a lot of history prior to this change, as this is a "take-over" of
> Issue 10836025 (http://chromiumcodereview.appspot.com/10836025).   The
> "lots of extra parameter passing" was discussed there, and the end decision
> was to go with it for now.
>
> As for needing render_process_id, there is one AudioRendererHost instance
> created and owned by each RenderProcessHost, so it will be trivial to plumb
> that in when it's needed for later changes (coming soon).
>
> Thanks for taking a look.  I'll keep you in the loop with the upcoming
> plumbing work.
>
> -Yuri
>
>
>
> On Thu, Oct 25, 2012 at 10:07 AM, <stevenjb@chromium.org> wrote:
>
>> I'm not at all familiar with the audio code, but when working on
>>
http://codereview.chromium.**org/11236052/<http://codereview.chromium.org/112...,
>> I needed both the render view id and
>> the render process id to associate a media stream with a tab. I don't see
>> any
>> references to the render process id here, is that already available?
>>
>> Also, again I am unfamiliar with the code, but it seems like we are
>> adding an
>> awful lot of extra parameter passing here. Is there maybe some other way
>> to
>> retrieve the associated render process / view id when it is needed?
>>
>>
>>
https://codereview.chromium.**org/11166002/<https://codereview.chromium.org/1...
>>
>
>

[jamesr, 2012-10-29 22:51:02]

http://codereview.chromium.org/11166002/diff/16045/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_impl.h (right):

http://codereview.chromium.org/11166002/diff/16045/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_impl.h:448:
DISALLOW_IMPLICIT_CONSTRUCTORS(WebRtcAudioDeviceImpl);
why are you changing these? The style guide seems to prefer
DISALLOW_COPY_AND_ASSIGN()

http://codereview.chromium.org/11166002/diff/16045/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_platform_audio_input_impl.h (right):

http://codereview.chromium.org/11166002/diff/16045/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_audio_input_impl.h:42: int
render_view_id,
you don't need this - you're passing in a WeakPtr to a PepperPluginDelegateImpl
which provides a RenderViewImpl* getter.  Just grab the ID off that pointer
if/when you need it

http://codereview.chromium.org/11166002/diff/16045/content/renderer/pepper/pe...
content/renderer/pepper/pepper_platform_audio_input_impl.h:70: explicit
PepperPlatformAudioInputImpl(int render_view_id);
Unnecessary

http://codereview.chromium.org/11166002/diff/16045/content/renderer/renderer_...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

http://codereview.chromium.org/11166002/diff/16045/content/renderer/renderer_...
content/renderer/renderer_webkitplatformsupport_impl.cc:617: return new
RendererWebAudioDeviceImpl(kUnknownRenderViewId, params, callback);
I think this is not quite the right way to plumb this.  When you create an audio
device, it shouldn't really care about what render view ID it's associated with
- you really only care when you are going to play sound (i.e. the
WebAudioDevice::start() call).

I think the way to plumb this is to look at how web sockets work.  The lifecycle
there is something like this:
1.) WebKit creates a WebSocketStreamHandle via
Platform::createSocketStreamHandle().  The object is constructed on the chromium
side
2.) when WebKit's ready to use the socket, it calls
WebFrameClient::willOpenSocketStream(WebSocketStreamHandle*).  The chromium
implementation of this uses this call to associated the implementation of
WebSocketStreamHandle with the context of the WebFrame (which is associated with
a view, etc).
3.) WebKit uses the WebSocketStreamHandle to do whatever it likes and the
chromium side has the metadata associations it needs.

We really do not want to make WebKit directly aware of RenderViewIds.  It's not
a concern of WebKit how these things are hooked up and it doesn't impact the
operation of WebKit at all.  Instead, we should keep metadata associations like
this contained on the chromium side and use callbacks to associate state that we
need.

[jamesr, 2012-10-29 23:01:09]

FYI, following the WebSockets pattern in the WebKit API should help you out
quite a bit with your layering issues on the WebKit side.  You can leave the
creation path alone and don't need to provide any additional context there and
can have the WebAudio code issue the WebFrameClient call on the correct context
at start() time