Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(249)

Issue 11359196: Associate audio streams with their source/destination RenderView. (Closed)

Created:
8 years, 1 month ago by miu
Modified:
8 years ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org, justinlin, jamesr
Visibility:
Public.

Description

Associate audio streams with their source/destination RenderView. 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. Previous attempt/approach: https://codereview.chromium.org/11166002/ Specifics: 1. Added two new IPC messages: AudioHostMsg_AssociateStreamWithProducer and AudioInputHostMsg_AssociateStreamWithConsumer. 2. Added "associate to render_view_id" IPC calls: PepperPlatformAudioInputImpl, PepperPlatformAudioOutputImpl, RenderAudioSourceProvider, RendererWebAudioDeviceImpl, WebRtcAudioRenderer. 3. AudioOutputDevice instances re-use the same stream IDs. 4. Removed AudioDeviceFactory and replaced test-injection scheme in AudioRendererMixerManager. Not in scope: 1. Associating RenderViews with audio input to WebRTC. 2. Associating RenderViews with streams produced by the AudioRendererMixer. 3. Speech Input (implementation is in the browser process, not the render process). Testing method: Confirm correct render_view_id shows up in debug logging when engaging the various audio code paths. Some of the many test sites used: 1. PPAPI (in and out): http://www.midomi.com/index.php?action=main.mic_check 2. WebMediaPlayer (e.g., <audio> and <video>): http://html5video.org/ 3. WebAudio API: http://chromium.googlecode.com/svn/trunk/samples/audio/convolution-effects.html 4. WebRTC (output only): http://apprtc.appspot.com/ BUG=3541, 64215, 153392 TEST=Enabled debug logging via --vmodule=audio_renderer_host=1,audio_input_renderer_host=1 and then engaged the various Chrome audio code paths to confirm correct render_view_id shows up in browser process. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170919

Patch Set 1 #

Patch Set 2 : Rebased; and numerous clean-ups. #

Total comments: 10

Patch Set 3 : Addressed Andrew's review comments. #

Total comments: 20

Patch Set 4 : Addressed Dale's comments. #

Total comments: 6

Patch Set 5 : Make pass of refptr by const ref in WebRtcAudioCapturer::SetCapturerSource. #

Patch Set 6 : Consolidate construct/init/destruct code in AudioOutputDeviceTest. #

Total comments: 15

Patch Set 7 : Restored AudioDeviceFactory. Created new RendererAudioOutputDevice. #

Total comments: 16

Patch Set 8 : RendererWebAudioDeviceImpl: Device is created at Start, destroyed at Stop. Plus, Andrew's latest c… #

Total comments: 9

Patch Set 9 : Fixed indentation in renderer_audio_output_device.cc #

Total comments: 2

Patch Set 10 : Rebased. Plus, removed CalledOnValidThread DCHECK from sampleRate() call since nothing mutates. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+471 lines, -193 lines) Patch
M content/browser/renderer_host/media/audio_input_renderer_host.h View 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/audio_input_renderer_host.cc View 1 2 3 2 chunks +10 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/audio_renderer_host.h View 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/audio_renderer_host.cc View 2 chunks +10 lines, -0 lines 0 comments Download
M content/common/media/audio_messages.h View 1 chunk +10 lines, -0 lines 0 comments Download
M content/content_renderer.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/media/audio_device_factory.h View 1 2 3 4 5 6 2 chunks +10 lines, -13 lines 0 comments Download
M content/renderer/media/audio_device_factory.cc View 1 2 3 4 5 6 2 chunks +5 lines, -5 lines 0 comments Download
M content/renderer/media/audio_input_message_filter.h View 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/media/audio_input_message_filter.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M content/renderer/media/audio_message_filter.h View 1 2 3 chunks +15 lines, -3 lines 0 comments Download
M content/renderer/media/audio_message_filter.cc View 1 2 3 4 5 6 4 chunks +43 lines, -23 lines 0 comments Download
M content/renderer/media/audio_renderer_mixer_manager.h View 1 2 3 chunks +6 lines, -0 lines 0 comments Download
M content/renderer/media/audio_renderer_mixer_manager.cc View 1 2 3 4 5 6 4 chunks +12 lines, -2 lines 0 comments Download
M content/renderer/media/audio_renderer_mixer_manager_unittest.cc View 1 2 5 chunks +17 lines, -28 lines 0 comments Download
M content/renderer/media/media_stream_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/media/render_audiosourceprovider.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
M content/renderer/media/render_audiosourceprovider.cc View 1 2 3 4 5 6 7 3 chunks +10 lines, -2 lines 0 comments Download
A content/renderer/media/renderer_audio_output_device.h View 1 2 3 4 5 6 7 1 chunk +58 lines, -0 lines 0 comments Download
A content/renderer/media/renderer_audio_output_device.cc View 1 2 3 4 5 6 7 8 1 chunk +63 lines, -0 lines 0 comments Download
M content/renderer/media/renderer_webaudiodevice_impl.h View 1 2 3 4 5 6 7 8 9 2 chunks +13 lines, -3 lines 0 comments Download
M content/renderer/media/renderer_webaudiodevice_impl.cc View 1 2 3 4 5 6 7 8 9 3 chunks +45 lines, -18 lines 0 comments Download
M content/renderer/media/webrtc_audio_capturer.h View 1 2 3 4 1 chunk +2 lines, -1 line 0 comments Download
M content/renderer/media/webrtc_audio_capturer.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/media/webrtc_audio_device_impl.cc View 1 1 chunk +0 lines, -1 line 0 comments Download
M content/renderer/media/webrtc_audio_device_unittest.cc View 1 4 chunks +8 lines, -5 lines 0 comments Download
M content/renderer/media/webrtc_audio_renderer.h View 1 2 3 4 5 6 7 4 chunks +7 lines, -3 lines 0 comments Download
M content/renderer/media/webrtc_audio_renderer.cc View 1 2 3 4 5 6 3 chunks +4 lines, -2 lines 0 comments Download
M content/renderer/pepper/pepper_platform_audio_input_impl.h View 2 chunks +5 lines, -1 line 0 comments Download
M content/renderer/pepper/pepper_platform_audio_input_impl.cc View 1 4 chunks +6 lines, -1 line 0 comments Download
M content/renderer/pepper/pepper_platform_audio_output_impl.h View 4 chunks +7 lines, -2 lines 0 comments Download
M content/renderer/pepper/pepper_platform_audio_output_impl.cc View 3 chunks +11 lines, -6 lines 0 comments Download
M content/renderer/pepper/pepper_plugin_delegate_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -1 line 0 comments Download
M content/renderer/render_view_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M content/test/webrtc_audio_device_test.cc View 1 1 chunk +0 lines, -1 line 0 comments Download
M media/audio/audio_output_device.h View 1 2 3 4 5 6 3 chunks +17 lines, -8 lines 0 comments Download
M media/audio/audio_output_device.cc View 1 2 3 4 5 6 7 chunks +29 lines, -37 lines 0 comments Download
M media/audio/audio_output_device_unittest.cc View 1 2 3 4 5 6 chunks +22 lines, -22 lines 0 comments Download

Messages

Total messages: 34 (0 generated)
miu
Here's the whole thing. I could split this up into separate changes, if necessary. Any ...
8 years, 1 month ago (2012-11-14 00:05:43 UTC) #1
miu
Dale/Chris, Here's the code! Please review. :-) Link to "the plan" (if anything looks crazy ...
8 years ago (2012-11-27 01:02:40 UTC) #2
scherkus (not reviewing)
https://codereview.chromium.org/11359196/diff/3001/content/common/media/audio_messages.h File content/common/media/audio_messages.h (right): https://codereview.chromium.org/11359196/diff/3001/content/common/media/audio_messages.h#newcode103 content/common/media/audio_messages.h:103: int /* render_view_id */) Seeing this just jogged my ...
8 years ago (2012-11-27 22:55:26 UTC) #3
miu
Andrew, Addressed your comments. PTAL. Thanks, Yuri https://codereview.chromium.org/11359196/diff/3001/content/common/media/audio_messages.h File content/common/media/audio_messages.h (right): https://codereview.chromium.org/11359196/diff/3001/content/common/media/audio_messages.h#newcode103 content/common/media/audio_messages.h:103: int /* ...
8 years ago (2012-11-28 07:26:20 UTC) #4
DaleCurtis
https://codereview.chromium.org/11359196/diff/11001/content/browser/renderer_host/media/audio_input_renderer_host.cc File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/11359196/diff/11001/content/browser/renderer_host/media/audio_input_renderer_host.cc#newcode286 content/browser/renderer_host/media/audio_input_renderer_host.cc:286: extra line. https://codereview.chromium.org/11359196/diff/11001/content/renderer/media/audio_message_filter.cc File content/renderer/media/audio_message_filter.cc (right): https://codereview.chromium.org/11359196/diff/11001/content/renderer/media/audio_message_filter.cc#newcode25 content/renderer/media/audio_message_filter.cc:25: : ...
8 years ago (2012-11-28 19:29:50 UTC) #5
miu
Thanks Dale. Addressed your comments. PTAL. -Yuri https://codereview.chromium.org/11359196/diff/11001/content/browser/renderer_host/media/audio_input_renderer_host.cc File content/browser/renderer_host/media/audio_input_renderer_host.cc (right): https://codereview.chromium.org/11359196/diff/11001/content/browser/renderer_host/media/audio_input_renderer_host.cc#newcode286 content/browser/renderer_host/media/audio_input_renderer_host.cc:286: On 2012/11/28 ...
8 years ago (2012-11-28 20:59:03 UTC) #6
scherkus (not reviewing)
this is pretty much LGTM w/ nits from me, although I do wonder if we ...
8 years ago (2012-11-28 21:55:01 UTC) #7
miu
https://codereview.chromium.org/11359196/diff/11003/content/renderer/media/webrtc_audio_capturer.h File content/renderer/media/webrtc_audio_capturer.h (right): https://codereview.chromium.org/11359196/diff/11003/content/renderer/media/webrtc_audio_capturer.h#newcode43 content/renderer/media/webrtc_audio_capturer.h:43: void SetCapturerSource(scoped_refptr<media::AudioCapturerSource> source); On 2012/11/28 21:55:01, scherkus wrote: > ...
8 years ago (2012-11-28 22:20:54 UTC) #8
DaleCurtis
lgtm https://codereview.chromium.org/11359196/diff/11003/media/audio/audio_output_device_unittest.cc File media/audio/audio_output_device_unittest.cc (right): https://codereview.chromium.org/11359196/diff/11003/media/audio/audio_output_device_unittest.cc#newcode109 media/audio/audio_output_device_unittest.cc:109: virtual void SetUp() OVERRIDE { On 2012/11/28 22:20:54, ...
8 years ago (2012-11-28 22:34:39 UTC) #9
scherkus (not reviewing)
https://codereview.chromium.org/11359196/diff/11003/media/audio/audio_output_device_unittest.cc File media/audio/audio_output_device_unittest.cc (right): https://codereview.chromium.org/11359196/diff/11003/media/audio/audio_output_device_unittest.cc#newcode109 media/audio/audio_output_device_unittest.cc:109: virtual void SetUp() OVERRIDE { On 2012/11/28 22:20:54, Yuri ...
8 years ago (2012-11-28 22:37:32 UTC) #10
miu
> Construct+Initialize into the ctor and Destruct into the dtor. Agreed, and done. :-)
8 years ago (2012-11-28 22:48:06 UTC) #11
Chris Rogers
I haven't had a chance to look through the entire CL, but here's one comment: ...
8 years ago (2012-11-29 11:14:32 UTC) #12
tommi (sloooow) - chröme
lgtm with a few requests as long as Andrew is happy. https://codereview.chromium.org/11359196/diff/3007/content/renderer/media/audio_message_filter.cc File content/renderer/media/audio_message_filter.cc (right): ...
8 years ago (2012-11-29 13:08:38 UTC) #13
tommi (sloooow) - chröme
On 2012/11/29 13:08:38, tommi wrote: > lgtm with a few requests as long as Andrew ...
8 years ago (2012-11-29 13:12:28 UTC) #14
tommi (sloooow) - chröme
https://codereview.chromium.org/11359196/diff/3007/content/renderer/media/render_audiosourceprovider.cc File content/renderer/media/render_audiosourceprovider.cc (right): https://codereview.chromium.org/11359196/diff/3007/content/renderer/media/render_audiosourceprovider.cc#newcode53 content/renderer/media/render_audiosourceprovider.cc:53: render_thread->GetIOMessageLoopProxy()); Chris, Henrik and I just talked about this. ...
8 years ago (2012-11-29 15:30:38 UTC) #15
scherkus (not reviewing)
On 2012/11/29 15:30:38, tommi wrote: > https://codereview.chromium.org/11359196/diff/3007/content/renderer/media/render_audiosourceprovider.cc > File content/renderer/media/render_audiosourceprovider.cc (right): > > https://codereview.chromium.org/11359196/diff/3007/content/renderer/media/render_audiosourceprovider.cc#newcode53 > ...
8 years ago (2012-11-30 02:10:09 UTC) #16
miu
Tommi/Chris, I made a number of changes. Please read my responses to your comments and ...
8 years ago (2012-11-30 02:40:35 UTC) #17
tommi (sloooow) - chröme
Thanks Yuri. Do you want to do the start() change (see below) in a separate ...
8 years ago (2012-11-30 06:51:30 UTC) #18
scherkus (not reviewing)
neato! few nits + q's https://codereview.chromium.org/11359196/diff/14041/content/renderer/media/audio_message_filter.cc File content/renderer/media/audio_message_filter.cc (right): https://codereview.chromium.org/11359196/diff/14041/content/renderer/media/audio_message_filter.cc#newcode116 content/renderer/media/audio_message_filter.cc:116: DelegateMap zombies; braaiiinnnss!!! https://codereview.chromium.org/11359196/diff/14041/content/renderer/media/render_audiosourceprovider.h ...
8 years ago (2012-11-30 22:09:43 UTC) #19
miu
Tommi/Andrew, Thanks for the comments. Andrew: Made changes per your comments. Tommi: I moved construction/destruction ...
8 years ago (2012-12-01 00:40:16 UTC) #20
scherkus (not reviewing)
lgtm!
8 years ago (2012-12-03 18:32:10 UTC) #21
DaleCurtis
lgtm % nits. https://codereview.chromium.org/11359196/diff/4079/content/renderer/media/audio_message_filter.cc File content/renderer/media/audio_message_filter.cc (right): https://codereview.chromium.org/11359196/diff/4079/content/renderer/media/audio_message_filter.cc#newcode116 content/renderer/media/audio_message_filter.cc:116: DelegateMap zombies; +1 https://codereview.chromium.org/11359196/diff/4079/content/renderer/media/audio_renderer_mixer_manager.cc File content/renderer/media/audio_renderer_mixer_manager.cc ...
8 years ago (2012-12-03 18:52:03 UTC) #22
miu
https://codereview.chromium.org/11359196/diff/4079/content/renderer/media/audio_renderer_mixer_manager.cc File content/renderer/media/audio_renderer_mixer_manager.cc (right): https://codereview.chromium.org/11359196/diff/4079/content/renderer/media/audio_renderer_mixer_manager.cc#newcode20 content/renderer/media/audio_renderer_mixer_manager.cc:20: sink_for_testing_(NULL) { On 2012/12/03 18:52:03, DaleCurtis wrote: > Are ...
8 years ago (2012-12-03 20:20:51 UTC) #23
Chris Rogers
lgtm - looks great! Thanks for taking the time to work through this with everyone. ...
8 years ago (2012-12-03 20:50:11 UTC) #24
miu
Charlie, Need OWNERS approval for addition to content/content_renderer.gpyi. Thanks, Yuri https://codereview.chromium.org/11359196/diff/4081/content/renderer/media/renderer_webaudiodevice_impl.cc File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/11359196/diff/4081/content/renderer/media/renderer_webaudiodevice_impl.cc#newcode83 ...
8 years ago (2012-12-03 22:39:14 UTC) #25
Charlie Reis
Rubber stamp LGTM on content/content_renderer.gypi. I'm excited to see what this will support!
8 years ago (2012-12-03 23:30:35 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/11359196/14083
8 years ago (2012-12-04 01:11:44 UTC) #27
commit-bot: I haz the power
Presubmit check for 11359196-14083 failed and returned exit status 1. Running presubmit commit checks ...
8 years ago (2012-12-04 01:12:02 UTC) #28
miu
Adding cdn@ for OWNERS approval for content/common/media/audio_messages.h.
8 years ago (2012-12-04 01:16:38 UTC) #29
Cris Neckar
lgtm for now on messages but please get someone from the security team to look ...
8 years ago (2012-12-04 01:17:10 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/11359196/14083
8 years ago (2012-12-04 01:20:11 UTC) #31
miu
On 2012/12/04 01:17:10, Cris Neckar wrote: > lgtm for now on messages but please get ...
8 years ago (2012-12-04 01:21:55 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/11359196/14083
8 years ago (2012-12-04 08:03:59 UTC) #33
commit-bot: I haz the power
8 years ago (2012-12-04 09:53:31 UTC) #34
Message was sent while issue was closed.
Change committed as 170919

Powered by Google App Engine
This is Rietveld 408576698