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

Issue 2834643002: audioproc_f with simulated mic analog gain (Closed)

Created:
3 years, 8 months ago by AleBzk
Modified:
3 years, 2 months ago
Reviewers:
aleloi, peah-webrtc
CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, Andrew MacDonald, aleloi, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

audioproc_f with simulated mic analog gain The gain suggested by AGC is optionally used in audioproc_f to simulate analog gain applied to the mic. The simulation is done by applying digital gain to the input samples. This functionality is optional and disabled by default. If an AECdump is provided and the mic gain simulation is enabled, an extra "level undo" step is performed to virtually restore the unmodified mic signal. Authors: alessiob, aleloi BUG=webrtc:7494

Patch Set 1 : set_stream_analog_level and stream_analog_level moved into parent class AudioProcessingSimulator #

Total comments: 13

Patch Set 2 : Comments from Alex addressed #

Total comments: 13

Patch Set 3 : comments addressed #

Total comments: 6

Patch Set 4 : AGC simulated gain #

Total comments: 66

Patch Set 5 : FakeRecordingDevice interface simplified, UTs fixes, logs verbosity-- #

Total comments: 52

Patch Set 6 : comments addressed #

Total comments: 47

Patch Set 7 : FakeRecordingDevice refactoring, minor comments addressed #

Total comments: 3

Patch Set 8 : FakeRecordingDevice: API simplified, UTs adapted #

Total comments: 7

Patch Set 9 : fake rec device boilerplate reduced, aec dump simulated analog gain logic moved #

Total comments: 50

Patch Set 10 : Merge + comments addressed #

Total comments: 30

Patch Set 11 : comments from Per addressed #

Total comments: 12

Patch Set 12 : agc api call seq, zero undo mic gain, mic level members simplified #

Total comments: 4

Patch Set 13 : minor changes #

Total comments: 6

Patch Set 14 : AEC dump + fake rec device bugfix #

Total comments: 20
Unified diffs Side-by-side diffs Delta from patch set Stats (+595 lines, -32 lines) Patch
M webrtc/modules/audio_processing/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +14 lines, -0 lines 0 comments Download
M webrtc/modules/audio_processing/test/aec_dump_based_simulator.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -2 lines 0 comments Download
M webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +10 lines, -19 lines 0 comments Download
M webrtc/modules/audio_processing/test/audio_processing_simulator.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +8 lines, -0 lines 2 comments Download
M webrtc/modules/audio_processing/test/audio_processing_simulator.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +56 lines, -1 line 2 comments Download
M webrtc/modules/audio_processing/test/audioproc_float.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +24 lines, -0 lines 0 comments Download
A webrtc/modules/audio_processing/test/fake_recording_device.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +77 lines, -0 lines 0 comments Download
A webrtc/modules/audio_processing/test/fake_recording_device.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +165 lines, -0 lines 14 comments Download
A webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +238 lines, -0 lines 2 comments Download
M webrtc/modules/audio_processing/test/wav_based_simulator.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +1 line, -1 line 0 comments Download
M webrtc/modules/audio_processing/test/wav_based_simulator.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +1 line, -9 lines 0 comments Download

Messages

Total messages: 60 (20 generated)
AleBzk
Hi Alex, This is a first patch set with some changes (incl. missing imports notified ...
3 years, 8 months ago (2017-04-21 09:43:46 UTC) #4
aleloi
What will happen if we always do the gain control level updating instead of passing ...
3 years, 8 months ago (2017-04-21 11:46:43 UTC) #5
aleloi
https://codereview.webrtc.org/2834643002/diff/20001/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2834643002/diff/20001/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc#newcode164 webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:164: // If so and the analog gain simulation is ...
3 years, 8 months ago (2017-04-21 11:52:29 UTC) #6
AleBzk
Hi Alex, Thanks a lot for your comments. PTAL, once you don't have further comments, ...
3 years, 8 months ago (2017-04-24 09:40:26 UTC) #7
aleloi
lgtm
3 years, 8 months ago (2017-04-24 11:48:28 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2834643002/40001
3 years, 8 months ago (2017-04-26 09:40:11 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/16458)
3 years, 8 months ago (2017-04-26 09:45:01 UTC) #12
AleBzk
I added Henrik, we need approval from one of the owners.
3 years, 8 months ago (2017-04-26 09:47:29 UTC) #14
hlundin-webrtc
I'm having difficulties understanding the logic. https://codereview.webrtc.org/2834643002/diff/40001/webrtc/modules/audio_processing/test/audio_processing_simulator.cc File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2834643002/diff/40001/webrtc/modules/audio_processing/test/audio_processing_simulator.cc#newcode132 webrtc/modules/audio_processing/test/audio_processing_simulator.cc:132: last_specified_microphone_level_ = settings_.simulate_mic_gain ...
3 years, 8 months ago (2017-04-26 12:11:37 UTC) #15
peah-webrtc
Some drive-by comments. https://codereview.webrtc.org/2834643002/diff/40001/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2834643002/diff/40001/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc#newcode163 webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:163: // If the AECdump does not ...
3 years, 8 months ago (2017-04-26 12:54:44 UTC) #17
AleBzk
Thanks for all your comments. I think it's best to discuss the changes offline before ...
3 years, 8 months ago (2017-04-26 14:19:33 UTC) #18
AleBzk
After our offline discussion, I made changes to this CL. I also took into account ...
3 years, 7 months ago (2017-05-02 14:53:25 UTC) #19
peah-webrtc
Thanks for the new draft! I added some comments. It is, however, a bit hard ...
3 years, 7 months ago (2017-05-02 21:27:33 UTC) #20
AleBzk
Finally here, PTAL
3 years, 7 months ago (2017-05-04 11:32:00 UTC) #22
aleloi
LG! Some comments in the unit test, though. https://codereview.webrtc.org/2834643002/diff/40001/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2834643002/diff/40001/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc#newcode163 webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:163: // ...
3 years, 7 months ago (2017-05-04 12:47:14 UTC) #27
peah-webrtc
Hi, Thanks for the new patch! I have added some comments. https://codereview.webrtc.org/2834643002/diff/100001/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): ...
3 years, 7 months ago (2017-05-05 06:28:41 UTC) #30
AleBzk
Thanks a lot for your comments. I addressed everything and also added the initial mic ...
3 years, 7 months ago (2017-05-05 12:20:19 UTC) #31
peah-webrtc
Hi, Thanks for the new patch. I have some more comments. https://codereview.webrtc.org/2834643002/diff/100001/webrtc/modules/audio_processing/test/audio_processing_simulator.cc File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): ...
3 years, 7 months ago (2017-05-05 20:25:22 UTC) #32
aleloi
Small quick comment response re build files and GN. More will come later. https://codereview.webrtc.org/2834643002/diff/120001/webrtc/modules/audio_processing/BUILD.gn File ...
3 years, 7 months ago (2017-05-08 09:46:49 UTC) #33
aleloi
https://codereview.webrtc.org/2834643002/diff/120001/webrtc/modules/audio_processing/BUILD.gn File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/2834643002/diff/120001/webrtc/modules/audio_processing/BUILD.gn#newcode675 webrtc/modules/audio_processing/BUILD.gn:675: rtc_source_set("fake_recording_device") { Additional arguments for modular targets: * GN ...
3 years, 7 months ago (2017-05-08 10:15:24 UTC) #34
peah-webrtc
https://codereview.webrtc.org/2834643002/diff/120001/webrtc/modules/audio_processing/BUILD.gn File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/2834643002/diff/120001/webrtc/modules/audio_processing/BUILD.gn#newcode675 webrtc/modules/audio_processing/BUILD.gn:675: rtc_source_set("fake_recording_device") { On 2017/05/08 09:46:49, aleloi wrote: > On ...
3 years, 7 months ago (2017-05-08 11:41:33 UTC) #35
aleloi
https://codereview.webrtc.org/2834643002/diff/120001/webrtc/modules/audio_processing/BUILD.gn File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/2834643002/diff/120001/webrtc/modules/audio_processing/BUILD.gn#newcode675 webrtc/modules/audio_processing/BUILD.gn:675: rtc_source_set("fake_recording_device") { On 2017/05/08 11:41:33, peah-webrtc wrote: > On ...
3 years, 7 months ago (2017-05-08 12:40:50 UTC) #36
peah-webrtc
https://codereview.webrtc.org/2834643002/diff/120001/webrtc/modules/audio_processing/BUILD.gn File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/2834643002/diff/120001/webrtc/modules/audio_processing/BUILD.gn#newcode675 webrtc/modules/audio_processing/BUILD.gn:675: rtc_source_set("fake_recording_device") { On 2017/05/08 12:40:49, aleloi wrote: > On ...
3 years, 7 months ago (2017-05-08 13:06:37 UTC) #37
AleBzk
Hi, Sorry for the delay and thanks a lot for your comments. PTAL. Alessio https://codereview.webrtc.org/2834643002/diff/120001/webrtc/modules/audio_processing/BUILD.gn ...
3 years, 7 months ago (2017-05-16 08:53:04 UTC) #38
peah-webrtc
Hi, Thanks for the new patch! I have added some comments. PTAL https://codereview.webrtc.org/2834643002/diff/120001/webrtc/modules/audio_processing/BUILD.gn File webrtc/modules/audio_processing/BUILD.gn ...
3 years, 7 months ago (2017-05-16 12:19:36 UTC) #39
AleBzk
Thank a lot for the comments! I think there is a point we should discuss ...
3 years, 7 months ago (2017-05-17 11:52:24 UTC) #41
peah-webrtc
Hi, Thanks for the new patch! I have some more comments. PTAL https://codereview.webrtc.org/2834643002/diff/140001/webrtc/modules/audio_processing/BUILD.gn File webrtc/modules/audio_processing/BUILD.gn ...
3 years, 7 months ago (2017-05-17 14:52:13 UTC) #42
AleBzk
Hi again, PTAL. I didn't directly reply to the past comments to avoid the risk ...
3 years, 7 months ago (2017-05-23 13:56:41 UTC) #43
peah-webrtc
https://codereview.webrtc.org/2834643002/diff/200001/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc (right): https://codereview.webrtc.org/2834643002/diff/200001/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc#newcode180 webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:180: fake_recording_device_->set_mic_level(msg.level()); On 2017/05/23 13:56:41, AleBzk wrote: > @Per: you ...
3 years, 7 months ago (2017-05-23 22:13:20 UTC) #44
AleBzk
Hi, Sorry for the belated answer to your comments. Following Per's feedback, we now have ...
3 years, 6 months ago (2017-06-22 10:16:01 UTC) #46
peah-webrtc
Hi, Thanks for the new patch! I have added some further comments. https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc File webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc ...
3 years, 5 months ago (2017-06-29 05:45:27 UTC) #47
AleBzk
Thanks Per! Getting there :) Sorry, I had an issue while merging for which I ...
3 years, 5 months ago (2017-06-29 11:43:37 UTC) #51
peah-webrtc
Hi, Thanks for the new patch! I have some more comments! https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_processing/test/audio_processing_simulator.cc File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): ...
3 years, 5 months ago (2017-06-29 22:04:00 UTC) #52
AleBzk
Hi Per, Comments addressed, two major points that might still be open: 1. Simplify fake ...
3 years, 4 months ago (2017-07-26 13:42:31 UTC) #53
peah-webrtc
Hi, Thanks for the patch. Here comes some more comments to be coupled with where ...
3 years, 4 months ago (2017-08-18 04:27:00 UTC) #54
AleBzk
Hi Per, As per our offline discussion, I've done the following changes: - float range ...
3 years, 4 months ago (2017-08-18 07:49:47 UTC) #55
peah-webrtc
Awesome! Thanks for the update! This is getting closer, but I don't think the analog ...
3 years, 4 months ago (2017-08-18 08:54:29 UTC) #56
AleBzk
Hi Per, Thanks for having caught a bug and my apologies for that. I fixed ...
3 years, 3 months ago (2017-09-04 12:02:04 UTC) #57
peah-webrtc
Thanks for the patch! I think it looks great! I have some minor further comments ...
3 years, 3 months ago (2017-09-15 09:36:21 UTC) #59
AleBzk
3 years, 3 months ago (2017-09-22 12:33:56 UTC) #60
Hi Per,

Thanks for your comments. I moved this CL to Gerrit and all the comments are
addressed in there (see https://webrtc-review.googlesource.com/c/src/+/2685).

Alessio

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right):

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.cc:148:
ap_->gain_control()->set_stream_analog_level(analog_mic_level_));
On 2017/09/15 09:36:20, peah-webrtc wrote:
> What about bundling line 148 with 143 using a conditional as
> ap_->gain_control()->set_stream_analog_level(settings_.aec_dump_input_filename
?
> *aec_dump_mic_level_:analog_mic_level_))

Done.

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/test/audio_processing_simulator.h (right):

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.h:185: 
On 2017/09/15 09:36:20, peah-webrtc wrote:
> Please remove the empty line on 185

Done.

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/test/fake_recording_device.cc (right):

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:74:
RTC_DCHECK_LE(number_of_samples, AudioFrame::kMaxDataSizeSamples);
On 2017/09/15 09:36:20, peah-webrtc wrote:
> Is this DCHECK really needed?
> since both samples_per_channel and num_channels are members of buffer (which
is
> of type AudioFrame), I think there is no need to check that they fulfill the
> requirements of AudioFrame (AudioFrame::kMaxDataSizeSamples))

Done.

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:77: const float
sample_f = data[i];
On 2017/09/15 09:36:20, peah-webrtc wrote:
> Why store data[i] in a local variable? It should be sufficient to use data[i]
> directly instead of using sample_f.

Done.

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:77: const float
sample_f = data[i];
On 2017/09/15 09:36:21, peah-webrtc wrote:
> sample_f -> sample. No need to specify the type in the name.

Acknowledged.

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:134:
RTC_DCHECK(worker_);
On 2017/09/15 09:36:20, peah-webrtc wrote:
> This should probably be a CHECK, since it is test code.

Fixed this and all the other DCHECKs. Thanks.

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:141: LOG(LS_INFO)
<< "simulate mic level update: " << level;
On 2017/09/15 09:36:21, peah-webrtc wrote:
> simulate -> Simulate

Done.

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:155:
RTC_DCHECK(worker_);
On 2017/09/15 09:36:20, peah-webrtc wrote:
> This should probably be a CHECK, since it is test code.

Done.

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:160:
RTC_DCHECK(worker_);
On 2017/09/15 09:36:21, peah-webrtc wrote:
> This should probably be a CHECK, since it is test code.

Done.

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc
(right):

https://codereview.webrtc.org/2834643002/diff/370001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc:211:
std::unique_ptr<const ChannelBuffer<float>> buff_orig =
On 2017/09/15 09:36:21, peah-webrtc wrote:
> auto as below?

I want a pointer to a const ChannelBuffer, and const auto would instead return a
const std::unique_ptr.

Powered by Google App Engine
This is Rietveld 408576698