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

Issue 2834643002: audioproc_f with simulated mic analog gain

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months ago by AleBzk
Modified:
4 days, 13 hours 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: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+580 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 0 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 5 chunks +41 lines, -1 line 1 comment 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 1 chunk +77 lines, -0 lines 1 comment 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 0 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 1 chunk +238 lines, -0 lines 1 comment 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: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 56 (19 generated)
AleBzk
Hi Alex, This is a first patch set with some changes (incl. missing imports notified ...
4 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 ...
4 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 ...
4 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, ...
4 months ago (2017-04-24 09:40:26 UTC) #7
aleloi
lgtm
4 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 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)
3 months, 4 weeks ago (2017-04-26 09:45:01 UTC) #12
AleBzk
I added Henrik, we need approval from one of the owners.
3 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 ...
3 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 ...
3 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 ...
3 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 ...
3 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 ...
3 months, 3 weeks ago (2017-05-02 21:27:33 UTC) #20
AleBzk
Finally here, PTAL
3 months, 2 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: // ...
3 months, 2 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): ...
3 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 ...
3 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): ...
3 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 ...
3 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 ...
3 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 ...
3 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 ...
3 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 ...
3 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 ...
3 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 ...
3 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 ...
3 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 ...
3 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 ...
3 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 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 ...
2 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 ...
1 month, 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 ...
1 month, 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): ...
1 month, 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 ...
3 weeks, 6 days 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 ...
4 days, 17 hours 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 ...
4 days, 14 hours ago (2017-08-18 07:49:47 UTC) #55
peah-webrtc
4 days, 13 hours ago (2017-08-18 08:54:29 UTC) #56
Awesome! Thanks for the update!
This is getting closer, but I don't think the analog AGC simulation works still
as before when running on an aecdump recording and the fake microphone is not
active.

Please have a look and do some tests on audioproc_f in order to verify that the
code in the CL is bitexact for that case compared to how it was before.

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

https://codereview.webrtc.org/2834643002/diff/350001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.cc:137:
ap_->gain_control()->set_stream_analog_level(analog_mic_level_));
Better, but this does still not work.
if settings_.simulate_mic_gain is false, aec_dump_mic_level_ is never passed to
set_stream_analog_level() since analog_mic_level_ is never set.

I would prefer a scheme with an explicit
set_stream_analog_level(aec_dump_mic_level_) that does not involve any
unnessessary intermediate variable when aecdump recordings are processed without
any simulated microphone gain, similarly to how the code constructs that I
proposed earlier does.

Also, please do some informal and exploratory tests (using audioproc_f) in order
to ensure that the new code really works as the former one when used for
simulation based on aecdum precordings. That would catch these kinds of errors.

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

https://codereview.webrtc.org/2834643002/diff/350001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.h:35: //
fake_mic.set_mic_level(170);
Please update the comments to comply with the api changes.

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

https://codereview.webrtc.org/2834643002/diff/350001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc:226: 
Please remove empty line after comment.
Sign in to reply to this message.

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