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

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

Created:
5 years ago by ajm
Modified:
5 years 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 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 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 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 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 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 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 ago (2014-08-12 01:47:35 UTC) #7
ajm
Tommi, could you take a look?
5 years ago (2014-08-13 00:39:08 UTC) #8
tommi (sloooow) - chröme
The CQ bit was checked by tommi@chromium.org
5 years ago (2014-08-13 08:11:08 UTC) #9
tommi (sloooow) - chröme
lgtm
5 years 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 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 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 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 ago (2014-08-13 14:07:00 UTC) #14
tommi (sloooow) - chröme
The CQ bit was checked by tommi@chromium.org
5 years ago (2014-08-13 15:29:12 UTC) #15
tommi (sloooow) - chröme
The CQ bit was unchecked by tommi@chromium.org
5 years ago (2014-08-13 15:29:30 UTC) #16
ajm
Thanks. It's a legitimate failure; I'm investigating.
5 years 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 ago (2014-08-13 21:40:51 UTC) #18
ajm
The CQ bit was checked by ajm@chromium.org
5 years 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 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 ago (2014-08-14 03:33:05 UTC) #21
ajm
The CQ bit was checked by ajm@chromium.org
5 years 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 ago (2014-08-14 03:35:13 UTC) #23
ajm
The CQ bit was unchecked by ajm@chromium.org
5 years ago (2014-08-14 17:01:40 UTC) #24
ajm
The CQ bit was checked by ajm@chromium.org
5 years 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 ago (2014-08-14 17:04:21 UTC) #26
commit-bot: I haz the power
5 years 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