Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(6)

Issue 420603004: Use the AudioProcessing float interface in MediaStreamAudioProcessor. (Closed)

Created:
5 years, 4 months ago by ajm
Modified:
5 years, 3 months ago
CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org, Henrik Grunell, no longer working on chromium
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Use the AudioProcessing float interface in MediaStreamAudioProcessor. Format conversions are now handled internally by AP, allowing us to to tear out the conversion code from MSAP. Instead, we use a simple FIFO to buffer the incoming audio into the 10 ms chunks AP requires. Add an AudioBus wrapper to get an array of channel pointers, since this is the type AP deals in. Add a few TODOs around two suspect areas: i) there appears to be an assumption on the sink type based on the input format, and ii) don't we want ProcessAndConsumeData() to output float data? BUG=400933 TESTED=Audio processing behaves as expected on a MacBook, using both 44.1 and 48 kHz as the render and capture rates (set through "Audio Midi Setup"). Audio sounds fine when processing is disabled through constraints. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289653

Patch Set 1 : #

Total comments: 5

Patch Set 2 : Add render_format_ and remove FIFO accessors. #

Total comments: 3

Patch Set 3 : Add MediaStreamAudioBus. #

Total comments: 4

Patch Set 4 : Remove mutable. #

Total comments: 1

Patch Set 5 : Cap the output buffers at 10 ms (previous behavior). #

Patch Set 6 : OnDataCallback can be called more than once. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+273 lines, -233 lines) Patch
M content/renderer/media/media_stream_audio_processor.h View 1 2 3 5 chunks +41 lines, -41 lines 0 comments Download
M content/renderer/media/media_stream_audio_processor.cc View 1 2 3 4 11 chunks +230 lines, -191 lines 0 comments Download
M content/renderer/media/webrtc_audio_capturer_unittest.cc View 1 2 3 4 5 1 chunk +2 lines, -1 line 2 comments Download

Messages

Total messages: 27 (0 generated)
ajm
Normally I'd get Henrik G and Shijing to review this, but since both are out, ...
5 years, 4 months ago (2014-08-06 03:59:11 UTC) #1
tommi (sloooow) - chröme
https://codereview.chromium.org/420603004/diff/50001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/420603004/diff/50001/content/renderer/media/media_stream_audio_processor.cc#newcode134 content/renderer/media/media_stream_audio_processor.cc:134: int channels() { return destination_->channels(); } make these const? ...
5 years, 4 months ago (2014-08-06 13:45:56 UTC) #2
ajm
Dale, could you have a look at the AudioBus change? (and if you like, the ...
5 years, 4 months ago (2014-08-07 04:11:37 UTC) #3
DaleCurtis
https://codereview.chromium.org/420603004/diff/70001/media/base/audio_bus.h File media/base/audio_bus.h (right): https://codereview.chromium.org/420603004/diff/70001/media/base/audio_bus.h#newcode96 media/base/audio_bus.h:96: float** channel_data() { return &channel_data_[0]; } This isn't okay ...
5 years, 4 months ago (2014-08-07 20:05:06 UTC) #4
ajm
https://codereview.chromium.org/420603004/diff/70001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/420603004/diff/70001/content/renderer/media/media_stream_audio_processor.cc#newcode254 content/renderer/media/media_stream_audio_processor.cc:254: capture_thread_checker_.CalledOnValidThread()); Turns out this check failed, I assume because ...
5 years, 4 months ago (2014-08-08 02:20:11 UTC) #5
DaleCurtis
I defer to tommi@ now since this is all a bit fuzzy to me now, ...
5 years, 3 months ago (2014-08-11 20:53:13 UTC) #6
ajm
Tommi, PTAL. https://codereview.chromium.org/420603004/diff/90001/content/renderer/media/media_stream_audio_processor.cc File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/420603004/diff/90001/content/renderer/media/media_stream_audio_processor.cc#newcode116 content/renderer/media/media_stream_audio_processor.cc:116: // Mutable to allow updating before a ...
5 years, 3 months ago (2014-08-12 01:47:35 UTC) #7
ajm
Tommi, could you take a look?
5 years, 3 months ago (2014-08-13 00:39:08 UTC) #8
tommi (sloooow) - chröme
The CQ bit was checked by tommi@chromium.org
5 years, 3 months ago (2014-08-13 08:11:08 UTC) #9
tommi (sloooow) - chröme
lgtm
5 years, 3 months ago (2014-08-13 08:11:08 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajm@chromium.org/420603004/130001
5 years, 3 months ago (2014-08-13 08:11:58 UTC) #11
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux ...
5 years, 3 months ago (2014-08-13 13:37:26 UTC) #12
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
5 years, 3 months ago (2014-08-13 14:06:59 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_swarming/builds/5794)
5 years, 3 months ago (2014-08-13 14:07:00 UTC) #14
tommi (sloooow) - chröme
The CQ bit was checked by tommi@chromium.org
5 years, 3 months ago (2014-08-13 15:29:12 UTC) #15
tommi (sloooow) - chröme
The CQ bit was unchecked by tommi@chromium.org
5 years, 3 months ago (2014-08-13 15:29:30 UTC) #16
ajm
Thanks. It's a legitimate failure; I'm investigating.
5 years, 3 months ago (2014-08-13 17:23:23 UTC) #17
ajm
On 2014/08/13 17:23:23, ajm wrote: > Thanks. It's a legitimate failure; I'm investigating. I had ...
5 years, 3 months ago (2014-08-13 21:40:51 UTC) #18
ajm
The CQ bit was checked by ajm@chromium.org
5 years, 3 months ago (2014-08-13 21:41:35 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajm@chromium.org/420603004/170001
5 years, 3 months ago (2014-08-13 21:43:44 UTC) #20
ajm
Trying to commit again because I'm fairly certain I know what the problem is. (See ...
5 years, 3 months ago (2014-08-14 03:33:05 UTC) #21
ajm
The CQ bit was checked by ajm@chromium.org
5 years, 3 months ago (2014-08-14 03:33:12 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajm@chromium.org/420603004/210001
5 years, 3 months ago (2014-08-14 03:35:13 UTC) #23
ajm
The CQ bit was unchecked by ajm@chromium.org
5 years, 3 months ago (2014-08-14 17:01:40 UTC) #24
ajm
The CQ bit was checked by ajm@chromium.org
5 years, 3 months ago (2014-08-14 17:01:46 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ajm@chromium.org/420603004/210001
5 years, 3 months ago (2014-08-14 17:04:21 UTC) #26
commit-bot: I haz the power
5 years, 3 months ago (2014-08-14 18:49:16 UTC) #27
Message was sent while issue was closed.
Committed patchset #6 (210001) as 289653

Powered by Google App Engine
This is Rietveld 408576698