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

Issue 139303016: Feed the render data to MediaStreamAudioProcessor and used AudioBus in render callback (Closed)

Created:
6 years, 11 months ago by no longer working on chromium
Modified:
4 years, 10 months ago
CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org
Visibility:
Public.

Description

Feed the render data to MediaStreamAudioProcessor and used AudioBus in render callback. This CL is mainly to feed the render data to MediaStreamAudioProcessor. But since it is better to have all the data format as media::AudioBus in chrome, so I changed the WebRtcAudioRendererSource::RenderData(int16*) to WebRtcAudioRendererSource::RenderData(media::AudioBus*), and also make MediaStreamAudioProcessor use WebRtcAudioRendererSource interface to get the render data. BUG=264611 TEST=content_unittests and AEC works when chrome ----enable-audio-track-processing Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252554

Patch Set 1 #

Patch Set 2 : fixed the unittests, ready for review now. #

Total comments: 7

Patch Set 3 : addressed Per's comments. #

Total comments: 13

Patch Set 4 : addressed Yuri's comments. #

Patch Set 5 : Fixed the win bots. #

Total comments: 21

Patch Set 6 : addressed Tommi's comments. #

Patch Set 7 : added WebRtcPlayoutDataSource and its sink interfaces. #

Patch Set 8 : fixed the bots. #

Total comments: 12

Patch Set 9 : check if aec is enabled in the unittest #

Patch Set 10 : rebased and added check the thread check on the destructor #

Total comments: 12

Patch Set 11 : #

Patch Set 12 : #

Total comments: 2

Patch Set 13 : comments were addressed #

Patch Set 14 : rebased #

Patch Set 15 : checked echo_control_mobile()->is_enabled()) for android and ios #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+202 lines, -208 lines) Patch
M content/renderer/media/media_stream_audio_processor.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +18 lines, -14 lines 0 comments Download
M content/renderer/media/media_stream_audio_processor.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +33 lines, -33 lines 0 comments Download
M content/renderer/media/media_stream_audio_processor_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +14 lines, -6 lines 0 comments Download
M content/renderer/media/webrtc_audio_capturer.h View 2 chunks +0 lines, -9 lines 0 comments Download
M content/renderer/media/webrtc_audio_capturer.cc View 1 2 3 2 chunks +2 lines, -22 lines 0 comments Download
M content/renderer/media/webrtc_audio_device_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +49 lines, -25 lines 0 comments Download
M content/renderer/media/webrtc_audio_device_impl.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +46 lines, -23 lines 3 comments Download
M content/renderer/media/webrtc_audio_device_unittest.cc View 1 2 3 4 5 6 7 8 9 5 chunks +7 lines, -17 lines 0 comments Download
M content/renderer/media/webrtc_audio_renderer.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +6 lines, -7 lines 0 comments Download
M content/renderer/media/webrtc_audio_renderer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +27 lines, -52 lines 0 comments Download

Messages

Total messages: 43 (4 generated)
no longer working on chromium
Tommi, could you please take a lead to review the CL? Per and Joi, it ...
6 years, 11 months ago (2014-01-17 13:52:17 UTC) #1
no longer working on chromium
Yuri, FYI, I take your suggestions on the render side. BTW, would you like to ...
6 years, 11 months ago (2014-01-17 15:02:07 UTC) #2
perkj_chrome
Just some nit comments. I am afraid I don't know the audio path well enough. ...
6 years, 11 months ago (2014-01-20 12:29:34 UTC) #3
no longer working on chromium
Thanks Per, PLAL. And also a gentle ping to other reviewers. SX https://codereview.chromium.org/139303016/diff/30001/content/renderer/media/webrtc_audio_device_impl.h File content/renderer/media/webrtc_audio_device_impl.h ...
6 years, 11 months ago (2014-01-23 13:02:30 UTC) #4
miu
Apologies for the delay in responding. On the whole, this is really nice! I'm not ...
6 years, 11 months ago (2014-01-24 22:27:18 UTC) #5
no longer working on chromium
Hi Yuri, Thanks very much for the review, and there is no problem on the ...
6 years, 10 months ago (2014-01-27 17:09:32 UTC) #6
miu
lgtm I'm a little concerned about the auto-adjustment of microphone volume. I'm not sure whether ...
6 years, 10 months ago (2014-01-27 23:46:47 UTC) #7
no longer working on chromium
On 2014/01/27 23:46:47, miu wrote: > lgtm > > I'm a little concerned about the ...
6 years, 10 months ago (2014-01-28 09:42:21 UTC) #8
no longer working on chromium
I know the rest of you guys are pretty busy, and probably I don't need ...
6 years, 10 months ago (2014-01-28 09:50:20 UTC) #9
tommi (sloooow) - chröme
https://codereview.chromium.org/139303016/diff/430001/content/renderer/media/media_stream_audio_processor_unittest.cc File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/139303016/diff/430001/content/renderer/media/media_stream_audio_processor_unittest.cc#newcode83 content/renderer/media/media_stream_audio_processor_unittest.cc:83: if (audio_processor->has_audio_processing()) { nit: no {} https://codereview.chromium.org/139303016/diff/430001/content/renderer/media/webrtc_audio_device_impl.cc File content/renderer/media/webrtc_audio_device_impl.cc ...
6 years, 10 months ago (2014-01-31 13:58:31 UTC) #10
no longer working on chromium
Tommi, PTAL. Thanks, SX https://codereview.chromium.org/139303016/diff/430001/content/renderer/media/media_stream_audio_processor_unittest.cc File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/139303016/diff/430001/content/renderer/media/media_stream_audio_processor_unittest.cc#newcode83 content/renderer/media/media_stream_audio_processor_unittest.cc:83: if (audio_processor->has_audio_processing()) { On 2014/01/31 ...
6 years, 10 months ago (2014-02-02 16:50:15 UTC) #11
no longer working on chromium
a gentle ping, Tommi, do you have time to take another look? Thanks, SX
6 years, 10 months ago (2014-02-04 12:55:34 UTC) #12
tommi (sloooow) - chröme
Just some terminology suggestions. https://codereview.chromium.org/139303016/diff/430001/content/renderer/media/webrtc_audio_device_impl.cc File content/renderer/media/webrtc_audio_device_impl.cc (right): https://codereview.chromium.org/139303016/diff/430001/content/renderer/media/webrtc_audio_device_impl.cc#newcode166 content/renderer/media/webrtc_audio_device_impl.cc:166: // Pass the render data ...
6 years, 10 months ago (2014-02-04 14:39:13 UTC) #13
no longer working on chromium
Tommi, I added new interfaces of WebRtcPlayoutDataSource and its sink, as we discussed offline. PTAL ...
6 years, 10 months ago (2014-02-05 15:18:36 UTC) #14
no longer working on chromium
Another gentle ping.
6 years, 10 months ago (2014-02-06 16:19:23 UTC) #15
tommi (sloooow) - chröme
sorry for the slow response https://codereview.chromium.org/139303016/diff/880001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (left): https://codereview.chromium.org/139303016/diff/880001/content/renderer/media/media_stream_audio_processor.cc#oldcode154 content/renderer/media/media_stream_audio_processor.cc:154: DCHECK(main_thread_checker_.CalledOnValidThread()); why removing? (it ...
6 years, 10 months ago (2014-02-06 16:47:01 UTC) #16
no longer working on chromium
Thanks, PTAL. https://codereview.chromium.org/139303016/diff/880001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (left): https://codereview.chromium.org/139303016/diff/880001/content/renderer/media/media_stream_audio_processor.cc#oldcode154 content/renderer/media/media_stream_audio_processor.cc:154: DCHECK(main_thread_checker_.CalledOnValidThread()); On 2014/02/06 16:47:01, tommi wrote: > ...
6 years, 10 months ago (2014-02-06 17:20:14 UTC) #17
tommi (sloooow) - chröme
just one more check about the ctor/dtor checks https://codereview.chromium.org/139303016/diff/880001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (left): https://codereview.chromium.org/139303016/diff/880001/content/renderer/media/media_stream_audio_processor.cc#oldcode154 content/renderer/media/media_stream_audio_processor.cc:154: DCHECK(main_thread_checker_.CalledOnValidThread()); ...
6 years, 10 months ago (2014-02-06 17:58:44 UTC) #18
no longer working on chromium
Tommi, I added back the thread check, PATL. FYI, I am going to land this ...
6 years, 10 months ago (2014-02-17 13:27:27 UTC) #19
tommi (sloooow) - chröme
https://codereview.chromium.org/139303016/diff/1190001/content/renderer/media/media_stream_audio_processor.h File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/139303016/diff/1190001/content/renderer/media/media_stream_audio_processor.h#newcode44 content/renderer/media/media_stream_audio_processor.h:44: // |audio_device| is used to register this class as ...
6 years, 10 months ago (2014-02-17 15:03:43 UTC) #20
no longer working on chromium
https://codereview.chromium.org/139303016/diff/1190001/content/renderer/media/media_stream_audio_processor.h File content/renderer/media/media_stream_audio_processor.h (right): https://codereview.chromium.org/139303016/diff/1190001/content/renderer/media/media_stream_audio_processor.h#newcode44 content/renderer/media/media_stream_audio_processor.h:44: // |audio_device| is used to register this class as ...
6 years, 10 months ago (2014-02-17 17:15:31 UTC) #21
tommi (sloooow) - chröme
lgtm. I'll drop by your desk for the is_enabled() question. I assume there's a simple ...
6 years, 10 months ago (2014-02-18 12:54:11 UTC) #22
no longer working on chromium
https://codereview.chromium.org/139303016/diff/1190001/content/renderer/media/media_stream_audio_processor_unittest.cc File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/139303016/diff/1190001/content/renderer/media/media_stream_audio_processor_unittest.cc#newcode85 content/renderer/media/media_stream_audio_processor_unittest.cc:85: const bool is_aec_enabled = ap && ap->echo_control_mobile()->is_enabled(); On 2014/02/18 ...
6 years, 10 months ago (2014-02-18 17:37:53 UTC) #23
no longer working on chromium
The CQ bit was checked by xians@chromium.org
6 years, 10 months ago (2014-02-20 12:55:37 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/139303016/1490001
6 years, 10 months ago (2014-02-20 12:55:57 UTC) #25
no longer working on chromium
The CQ bit was unchecked by xians@chromium.org
6 years, 10 months ago (2014-02-20 20:25:51 UTC) #26
no longer working on chromium
The CQ bit was checked by xians@chromium.org
6 years, 10 months ago (2014-02-21 09:58:44 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/139303016/1800001
6 years, 10 months ago (2014-02-21 09:59:05 UTC) #28
commit-bot: I haz the power
Change committed as 252554
6 years, 10 months ago (2014-02-21 15:00:42 UTC) #29
brucedawson
Apologies for commenting on a CL two years after it was submitted, but /analyze 2015 ...
4 years, 10 months ago (2016-01-30 18:32:07 UTC) #31
tommi (sloooow) - chröme
https://codereview.chromium.org/139303016/diff/1800001/content/renderer/media/webrtc_audio_device_impl.cc File content/renderer/media/webrtc_audio_device_impl.cc (right): https://codereview.chromium.org/139303016/diff/1800001/content/renderer/media/webrtc_audio_device_impl.cc#newcode147 content/renderer/media/webrtc_audio_device_impl.cc:147: int16* audio_data = &render_buffer_[0]; On 2016/01/30 18:32:07, brucedawson wrote: ...
4 years, 10 months ago (2016-02-01 08:47:53 UTC) #33
henrika (OOO until Aug 14)
Sure, will take a look and get back.
4 years, 10 months ago (2016-02-01 08:54:08 UTC) #34
henrika (OOO until Aug 14)
https://codereview.chromium.org/139303016/diff/1800001/content/renderer/media/webrtc_audio_device_impl.cc File content/renderer/media/webrtc_audio_device_impl.cc (right): https://codereview.chromium.org/139303016/diff/1800001/content/renderer/media/webrtc_audio_device_impl.cc#newcode147 content/renderer/media/webrtc_audio_device_impl.cc:147: int16* audio_data = &render_buffer_[0]; Added DCHECK_EQ(accumulated_audio_frames, audio_bus->frames()) << "OOOOOPS"; ...
4 years, 10 months ago (2016-02-01 14:46:46 UTC) #35
tommi (sloooow) - chröme
On 2016/02/01 14:46:46, henrika wrote: > https://codereview.chromium.org/139303016/diff/1800001/content/renderer/media/webrtc_audio_device_impl.cc > File content/renderer/media/webrtc_audio_device_impl.cc (right): > > https://codereview.chromium.org/139303016/diff/1800001/content/renderer/media/webrtc_audio_device_impl.cc#newcode147 > ...
4 years, 10 months ago (2016-02-01 15:35:06 UTC) #36
henrika (OOO until Aug 14)
Nice find, that seems to indicate that the while loop is not required. I will ...
4 years, 10 months ago (2016-02-01 15:40:30 UTC) #37
henrika (OOO until Aug 14)
+Henrik Lundin (hlundin@)
4 years, 10 months ago (2016-02-01 15:41:17 UTC) #40
brucedawson
On 2016/02/01 15:40:30, henrika wrote: > Nice find, that seems to indicate that the while ...
4 years, 10 months ago (2016-02-01 17:45:24 UTC) #41
hlundin-chromium
On 2016/02/01 15:40:30, henrika wrote: > Nice find, that seems to indicate that the while ...
4 years, 10 months ago (2016-02-15 14:16:54 UTC) #42
tommi (sloooow) - chröme
4 years, 10 months ago (2016-02-15 15:06:35 UTC) #43
Message was sent while issue was closed.
On 2016/02/15 14:16:54, hlundin1 wrote:
> On 2016/02/01 15:40:30, henrika wrote:
> > Nice find, that seems to indicate that the while loop is not required.
> > I will check with hlundin@ and upload a change after that.
> 
> I don't know anything about this code. Sorry.

The question for you was more for the code in VoE.  However, as per comment #36
I found that these assumptions are already being asserted inside of webrtc and
that's what we wanted to know.

Powered by Google App Engine
This is Rietveld 408576698