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

Issue 2834643002: audioproc_f with simulated mic analog gain

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by AleBzk
Modified:
5 days, 7 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 (OOOtoJul3), kwiberg-webrtc, minyue-webrtc, the sun (OOO until July 24th), 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: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+585 lines, -35 lines) Patch
M webrtc/modules/audio_processing/BUILD.gn View 1 2 3 4 5 6 7 8 4 chunks +13 lines, -0 lines 0 comments Download
M webrtc/modules/audio_processing/test/aec_dump_based_simulator.h View 1 2 3 2 chunks +3 lines, -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 4 chunks +15 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 5 chunks +8 lines, -1 line 0 comments Download
M webrtc/modules/audio_processing/test/audio_processing_simulator.cc View 1 2 3 4 5 6 7 8 5 chunks +50 lines, -1 line 2 comments Download
M webrtc/modules/audio_processing/test/audioproc_float.cc View 1 2 3 4 5 6 7 8 4 chunks +28 lines, -2 lines 0 comments Download
A webrtc/modules/audio_processing/test/fake_recording_device.h View 1 2 3 4 5 6 7 8 1 chunk +90 lines, -0 lines 2 comments Download
A webrtc/modules/audio_processing/test/fake_recording_device.cc View 1 2 3 4 5 6 7 8 1 chunk +138 lines, -0 lines 2 comments Download
A webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +235 lines, -0 lines 0 comments Download
M webrtc/modules/audio_processing/test/wav_based_simulator.h View 3 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 4 chunks +4 lines, -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: 46 (16 generated)
AleBzk
Hi Alex, This is a first patch set with some changes (incl. missing imports notified ...
2 months, 1 week 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 ...
2 months, 1 week 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 ...
2 months, 1 week 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, ...
2 months ago (2017-04-24 09:40:26 UTC) #7
aleloi
lgtm
2 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
2 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)
2 months ago (2017-04-26 09:45:01 UTC) #12
AleBzk
I added Henrik, we need approval from one of the owners.
2 months ago (2017-04-26 09:47:29 UTC) #14
hlundin-webrtc (OOOtoJul3)
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 ...
2 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 ...
2 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 ...
2 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 ...
1 month, 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 ...
1 month, 3 weeks ago (2017-05-02 21:27:33 UTC) #20
AleBzk
Finally here, PTAL
1 month, 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: // ...
1 month, 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): ...
1 month, 3 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 ...
1 month, 3 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): ...
1 month, 3 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 ...
1 month, 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 ...
1 month, 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 ...
1 month, 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 ...
1 month, 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 ...
1 month, 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 ...
1 month, 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 ...
1 month, 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 ...
1 month, 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 ...
1 month, 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 ...
1 month 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 ...
1 month ago (2017-05-23 22:13:20 UTC) #44
AleBzk
5 days, 7 hours ago (2017-06-22 10:16:01 UTC) #46
Hi,

Sorry for the belated answer to your comments.

Following Per's feedback, we now have the fake recording device implementations
in a single file, still with dedicated classes. Much less boilerplate. Thanks!

And I also moved the logic for the simulated analog gain when an AEC dump is
used as input.

I tested both with wav and AEC dumps, and it works as expected. Interesting to
see that, with an AEC dump, the level undo op leads to having AGC suggesting the
same mic levels logged in the AEC dump :)

Alessio

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

https://codereview.webrtc.org/2834643002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:180:
fake_recording_device_->set_mic_level(msg.level());
On 2017/05/23 22:13:20, peah-webrtc wrote:
> On 2017/05/23 13:56:41, AleBzk wrote:
> > @Per: you asked to move this to the parent class and store the level from
the
> > AEC dump into a local variable.
> > 
> > First of all, note that I'm not using local variables anymore but directly
the
> > fake recording device (which is a protected member of the parent class).
Last
> > time you reviewed this CL, you commented on an old PS using variables.
> > 
> > Back to your main point. If we move this logic to the parent class, then the
> > latter needs sth like "if (aec dump case)", which I don't like much because
> for
> > the AEC dump case we have a dedicated implementation of the abstract class.
> > 
> > In this way instead, the parent class just does the stuff common to WAV and
> > AECdump, whereas the additional features specific of each case are done in
> > WavBasedSimulator and AecDumpBasedSimulator respectively.
> > 
> > Finally, note that msg.level() has two different applications depending on
> > whether the analog mic is simulated. To me, it has to be
AecDumpBasedSimulator
> > that knows what to change in the simulation.
> > 
> > However, I understand that to me everything looks clear, but it could be
> > different for the reader. So, if you still see this confusing I propose the
> > following changes:
> > - in the parent class AudioProcessingSimulator, I add "rtc::Optional<int>
> > aec_dump_mic_level_" as protected member
> > - AecDumpBasedSimulator always sets its value in there
> > - AudioProcessingSimulator uses aec_dump_mic_level_ if available
> > - since WavBasedSimulator doesn't set it, there's no need of "if (aec dump)"
> > 
> > WDYT?
> 
> What I don't like with this approach is that it gives the impression that the
> fake_recording_device actually modifies the level in
> audio_processing_simulator.cc even though it is specified not to simulate the
> mic gain. Furthermore, in order to see this that this really is not the case
I'd
> need to dig fairly deep into fake_recording_device.
> 
> I see your point about that the AudioProcessingSimulator code should not need
to
> know whether you are using a wav or an aecdump based simulation. It is
> definitely valid but I don't think there is anyway around that in this case.
The
> latest patch actually also has such a dependency but in a hidden way as the
> fake_recording_device knows what kind of simulation is running. I'd rather
make
> that more explicit.
> 
> What about something similar to the following code:
> 
> void AecDumpBasedSimulator::PrepareProcessStreamCall() {
> ...
> 
> level_ = rtc::Optional<int>(msg.level());
> ...
> }
> 
> 
> class AudioProcessingSimulator {
> ...
> protected:
> rtc::Optional<int> level_;
> ...
> };
> 
> void AudioProcessingSimulator::ProcessStream(bool fixed_interface) {
>     // Optionally use the fake recording device to simulate analog gain.
>     RTC_DCHECK(fake_recording_device_);
>     if (settings_.simulate_mic_gain) {
>       if (fixed_interface) {
>         fake_recording_device_->SimulateAnalogGain(level_, &fwd_frame_);
>       } else {
>         fake_recording_device_->SimulateAnalogGain(level_, in_buf_.get());
>       }
>     
>
ap_->gain_control()->set_stream_analog_level(fake_recording_device_->mic_level()
> : level_));
>    }
>   else {
>    // Use the level 100 as default if no other level has been externally
> specified.
>    ap_->gain_control()->set_stream_analog_level(level_ ? * level_ : 100));
>  }
>   ...
>     // Process the current audio frame.
> ...
>     // Store the mic level suggested by AGC if required.
>    int new_level = ap_->gain_control()->stream_analog_level();
>    if (settings_.simulate_mic_gain) {
>      fake_recording_device_->set_mic_level(new_level);
>    }
> ..
> }
> 
> 
> 
> 

Done.

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

https://codereview.webrtc.org/2834643002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_rec_device_identity.h:11: #ifndef
WEBRTC_MODULES_AUDIO_PROCESSING_TEST_FAKE_REC_DEVICE_IDENTITY_H_
On 2017/05/23 22:13:20, peah-webrtc wrote:
> Sorry, but this is not what I meant. Please put these implementations in the
> fake_recording_device.cc file instead. This approach adds a lot of boilerplate
> code for something which is quite small. What I meant was that you instead of
> the method pointers should use interfaces and only implement them locally.
> 
> When you move them into separate files there is a lot of boilerplate code,
about
> 60 lines for each.
> Previously each method for this was 5 lines long and with local interfaces,
you
> should have something fairly similar.
> 
> I think that until we really put something substantial in these
implementations
> which show that they become so large as to warrant a separate file, we should
> simply put the implementations inside the fake_recording_device.cc  file.

Done.

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

https://codereview.webrtc.org/2834643002/diff/200001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.h:46: class
FakeRecordingDevice {
On 2017/05/23 22:13:20, peah-webrtc wrote:
>  I really think we should put as little implementation as possible into this
> class now, i.e., make it as small and simple as possible. The reason for that
is
> that we don't really know how to simulate this. 
> 
> I'm open to having one or two simple default implementations just to show how
it
> is supposed to work but I don't think it makes sense to do advanced
> implementations in a specific way. For instance, it is fairly clear to me that
> the current approach won't be sufficient to simulate a microphone with a noise
> suppressor (which we have on Macbooks), beamformers, or a microphone with any
> type of memory in its clipping behavior. Until we can rule out that we don't
> need such an implementation of the simulation I don't think we should have a
> code design that enforces that, unless that code is very simple.
> 
> Why not do this as simple as possible, and not more than that? It will be much
> easier to discuss the implementation once we really know what is needed to
> simulate the device.

Done.

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

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.cc:130:
fake_recording_device_->set_mic_level(*aec_dump_mic_level_);
Line 130 overrides what is set in lines 145-147.

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.cc:132: }
About lines 119-132: the AEC dump analog gain simulation logic has been moved
here following Per's request.

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

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:44: };
This is the class to be implemented for each simulated recording device.
The implementations are below in the anon ns.

Note that FakeRecordingDeviceWorker cannot be defined in the anon ns, because
it's needed in fake_recording_device.h.

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:57: void
ModifySampleFloat(float* sample) override {}
This is a recording device that does nothing.

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

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.h:27: class
FakeRecordingDeviceWorker;
Forward declaration for the FakeRecordingDeviceWorker abstract class, which is
defined in audio_processing_simulator.cc. The same file also contains
FakeRecordingDeviceWorker implementations (in the anon ns).

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.h:34: //     255,
FakeRecordingDevice.DeviceKind.LINEAR);
I fixed this as follows:

// FakeRecordingDevice fake_mic(
//     255, FakeRecordingDevice.DeviceKind.LINEAR);

Will upload in the next PS once I get your comments.
Sign in to reply to this message.

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