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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months ago by AleBzk
Modified:
3 weeks, 2 days 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(BackIn2018March), 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
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 60 (20 generated)
AleBzk
Hi Alex, This is a first patch set with some changes (incl. missing imports notified ...
6 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 ...
6 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 ...
6 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, ...
6 months ago (2017-04-24 09:40:26 UTC) #7
aleloi
lgtm
6 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
5 months, 4 weeks 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)
5 months, 4 weeks ago (2017-04-26 09:45:01 UTC) #12
AleBzk
I added Henrik, we need approval from one of the owners.
5 months, 4 weeks 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 ...
5 months, 4 weeks 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 ...
5 months, 4 weeks 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 ...
5 months, 4 weeks 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 ...
5 months, 3 weeks 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 ...
5 months, 3 weeks ago (2017-05-02 21:27:33 UTC) #20
AleBzk
Finally here, PTAL
5 months, 3 weeks 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: // ...
5 months, 3 weeks 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): ...
5 months, 2 weeks 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 ...
5 months, 2 weeks 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): ...
5 months, 2 weeks 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 ...
5 months, 2 weeks 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 ...
5 months, 2 weeks 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 ...
5 months, 2 weeks 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 ...
5 months, 2 weeks 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 ...
5 months, 2 weeks 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 ...
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 months, 1 week 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 ...
5 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 ...
5 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 ...
4 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 months, 3 weeks 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 months, 3 weeks 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 months, 3 weeks 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 ...
2 months, 4 weeks 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 ...
2 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 ...
2 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 ...
2 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 ...
1 month, 2 weeks 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 ...
1 month, 1 week ago (2017-09-15 09:36:21 UTC) #59
AleBzk
1 month 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa