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

Issue 2834643002: audioproc_f with simulated mic analog gain

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by AleBzk
Modified:
5 days, 2 hours ago
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: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+656 lines, -33 lines) Patch
M webrtc/modules/audio_processing/BUILD.gn View 1 2 3 4 5 6 7 4 chunks +17 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 4 chunks +25 lines, -17 lines 2 comments Download
M webrtc/modules/audio_processing/test/audio_processing_simulator.h View 1 2 3 4 5 6 4 chunks +7 lines, -1 line 0 comments Download
M webrtc/modules/audio_processing/test/audio_processing_simulator.cc View 1 2 3 4 5 6 5 chunks +35 lines, -1 line 0 comments Download
M webrtc/modules/audio_processing/test/audioproc_float.cc View 1 2 3 4 5 6 4 chunks +28 lines, -2 lines 0 comments Download
A webrtc/modules/audio_processing/test/fake_rec_device_identity.h View 1 2 3 4 5 6 1 chunk +32 lines, -0 lines 1 comment Download
A webrtc/modules/audio_processing/test/fake_rec_device_identity.cc View 1 2 3 4 5 6 1 chunk +26 lines, -0 lines 0 comments Download
A webrtc/modules/audio_processing/test/fake_rec_device_linear.h View 1 2 3 4 5 6 1 chunk +33 lines, -0 lines 0 comments Download
A webrtc/modules/audio_processing/test/fake_rec_device_linear.cc View 1 2 3 4 5 6 1 chunk +60 lines, -0 lines 0 comments Download
A webrtc/modules/audio_processing/test/fake_recording_device.h View 1 2 3 4 5 6 7 1 chunk +96 lines, -0 lines 1 comment Download
A webrtc/modules/audio_processing/test/fake_recording_device.cc View 1 2 3 4 5 6 7 1 chunk +92 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 1 chunk +197 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: 44 (15 generated)
AleBzk
Hi Alex, This is a first patch set with some changes (incl. missing imports notified ...
1 month, 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 ...
1 month, 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 ...
1 month, 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, ...
1 month ago (2017-04-24 09:40:26 UTC) #7
aleloi
lgtm
1 month 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
1 month 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)
1 month ago (2017-04-26 09:45:01 UTC) #12
AleBzk
I added Henrik, we need approval from one of the owners.
1 month 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 ...
1 month 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 ...
1 month 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 ...
1 month 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 weeks, 5 days 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 weeks, 5 days ago (2017-05-02 21:27:33 UTC) #20
AleBzk
Finally here, PTAL
3 weeks, 3 days 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 weeks, 3 days 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 weeks, 2 days 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 weeks, 2 days 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 weeks, 2 days 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 ...
2 weeks, 6 days 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 ...
2 weeks, 6 days 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 ...
2 weeks, 6 days 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 ...
2 weeks, 6 days 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 ...
2 weeks, 6 days 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 week, 5 days 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 week, 5 days 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 week, 4 days 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 week, 4 days 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 days, 10 hours ago (2017-05-23 13:56:41 UTC) #43
peah-webrtc
5 days, 2 hours ago (2017-05-23 22:13:20 UTC) #44
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 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);
   }
..
}

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_
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.

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 {
 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.
Sign in to reply to this message.

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