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

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:
38 minutes 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: 44

Patch Set 10 : Merge + comments addressed #

Total comments: 18
Unified diffs Side-by-side diffs Delta from patch set Stats (+556 lines, -32 lines) Patch
M webrtc/modules/audio_processing/BUILD.gn View 1 2 3 4 5 6 7 8 9 5 chunks +14 lines, -0 lines 3 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 9 4 chunks +13 lines, -19 lines 1 comment Download
M webrtc/modules/audio_processing/test/audio_processing_simulator.h View 1 2 3 4 5 6 7 8 9 4 chunks +6 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 5 chunks +44 lines, -1 line 4 comments Download
A webrtc/modules/audio_processing/test/fake_recording_device.h View 1 2 3 4 5 6 7 8 9 1 chunk +90 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 1 chunk +142 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 1 chunk +242 lines, -0 lines 8 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 6 7 8 9 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: 51 (19 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, 3 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, 3 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, 3 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, 3 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, 3 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, 2 weeks 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, 2 weeks 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
Hi, Sorry for the belated answer to your comments. Following Per's feedback, we now have ...
1 week 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 ...
6 hours, 36 minutes ago (2017-06-29 05:45:27 UTC) #47
AleBzk
38 minutes ago (2017-06-29 11:43:37 UTC) #51
Thanks Per!

Getting there :)

Sorry, I had an issue while merging for which I had to delete a few patch sets.
My revised code is in https://codereview.webrtc.org/2834643002/#ps290001

Alessio

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

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:11: #include
<algorithm>
On 2017/06/29 05:45:26, peah-webrtc wrote:
> Why was algorithm and utility added to the includes? Are those required by the
> former code?

Good catch! Thanks!

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:19: #include
"webrtc/modules/audio_processing/test/fake_recording_device.h"
On 2017/06/29 05:45:26, peah-webrtc wrote:
> This include is not needed, right?

Yet another good catch :)

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:89:
fake_recording_device_(new FakeRecordingDevice(
On 2017/06/29 05:45:27, peah-webrtc wrote:
> You don't need to create this dynamically. Since you have it in the
initializer
> list, you can instead just initialized it without dynamic allocation.

Thanks. I recalled why I added the member as a unique pointer. I wanted forward
declaration in the header file. But I guess the option you suggest is preferred
in the end.

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_);
On 2017/06/29 05:45:26, peah-webrtc wrote:
> As I commented on a previous patch, I definitely don't think that the
> fake_recording device should be involved when the mic gain is not simulated. 
> 
> If you avoid that, you can skip the set_mic_level method. 

Thanks. Sorry for having kept this. It's up to us deciding if this could make
sense.
My view is that, when an AEC dump is used as input, AGC should behave as close
as possible as in the recorded call. This can be done by informing AGC on the
mic gain that was set during the call.

But I also understand that, since no analog mic gain is simulated, there's not
much to observe.

I'll remove as you suggest.

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.cc:138:
fake_recording_device_->SimulateAnalogGain(&fwd_frame_);
On 2017/06/29 05:45:26, peah-webrtc wrote:
> Why not just add the optional aec_dump_mic_level_ as an input to
> SimulateAnalogGain? That would eliminate the need for the lines 119-132.

I'd prefer as it is now, otherwise we have to add nested "if"s since we have 4
combinations for (fixed_interface, aec_dum_input && simulate_gain).

Also, from a design perspective, either we pass all the parameters (i.e., undo
mic level and mic level) to SimulateAnalogGain() or just the buffer to process.

WDYT?

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.cc:147:
fake_recording_device_->mic_level()));
On 2017/06/29 05:45:27, peah-webrtc wrote:
> This is something that I think complicates the code quite a bit. As I
commented
> on a previous patch, I don't think that the fake recording device should at
all
> be involved with the mic_level if the mic gain should not be simulated.
> 
> As it is now, it seems from this line as if the fake_recording_device is
> choosing the mic level, instead of that the mic level in the aec_dump is
passed.

What happens during a call is that AGC suggests a gain but for many different
reasons, the suggestion can be overridden (e.g., the user manually sets the mic
gain via OS UI). I haven't verified if AGC makes use of the actual mic gain to
suggest a new one or if it uses the sequence of suggested gains blindly. That's
why I think we'd better to tell AGC the actual mic gain.

I understand that you find this confusing, but this is an AGC API issue - and in
fact we planned to eliminate the "set_stream_analog_level, ProcessStream,
stream_analog_level" sequence of invocations (error prone). My suggestion is
that until the AGC API remains as it is, we keep this call, maybe adding more
details in the comment.

WDYT?

> Furthermore, verifying that that is not the case is not that straightforward.
> 
> Why not do as I proposed in a comment on a previous patch, and pass in 
> *aec_dump_mic_level_ directly.

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.cc:163: // Store
the mic level suggested by AGC if required.
On 2017/06/29 05:45:27, peah-webrtc wrote:
> The comment "if required" seems out of place. Could you please clarify it, or
> rephrase it?

Done.

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:21: // Abstract
class for the different fake recording devices.
On 2017/06/29 05:45:27, peah-webrtc wrote:
> The scheme for the clipping and analog gain is quite intricate, but regardless
> of that I don't think it is general enough to scale. Therefore, I'd propose
(as
> I previously suggested) to have a much simpler implementation now until the
real
> simulation work of the recording device is started, as the implementation
herein
> is currently only a placeholder functionality for this.
> 
> My main concerns are the clipping. As it is now, that is hardcoded in the
parent
> class but we have discussed clipping solutions which don't comply to the
scheme
> that is hardcoded. How do you envision that to fit into the framework below? 

Thanks for these concerns.

First, to improve the clarity, I moved constants and the hard clipping functions
in the anon namespace.
It's good to have them somewhere to avoid code duplication.

Then note that each implementation of FakeRecordingDeviceWorker can implement
its clipping strategy - not obliged to use ClipSampleInt16/ClipSampleFloat.
Hence, I find the current design extremely flexible and also compact. In fact,
FakeRecordingDeviceLinear::ModifySampleFloat() is just 5 lines of code excl.
comments.

Also note that we need ModifySampleInt16/ModifySampleFloat mainly because of the
client code.
Another way is to only implement the float interface and scale in [-2^15, 2^15 -
1] afterwards for int16.

With that being said, if you find that having moved code to the anon namespace
is still insufficient, could you then be more specific when you say that we
could have a much simpler implementation?

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:69: float sample_f
= static_cast<float>(*sample);
On 2017/06/29 05:45:27, peah-webrtc wrote:
> You don't need to do the cast here.

Done.

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:74: sample_f *
static_cast<float>(mic_level_) / static_cast<float>(
On 2017/06/29 05:45:27, peah-webrtc wrote:
> Same thing here, no casts are needed.

Done.

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:74: sample_f *
static_cast<float>(mic_level_) / static_cast<float>(
On 2017/06/29 05:45:27, peah-webrtc wrote:
> There is a bug here I think. A float is  passed to ClipSampleInt16 but since
> that parameter is an int16_t I guess the value could be saturated already when
> the parameter is passed. I thought the compiler would warn for that though.

Right, thanks. Using goma I missed the compiler warnings.
I tried the code, clipping already occurs just by casting.
However, to avoid issues, I changed ClipSampleInt16 into ClipSampleFloatToInt16,
with the latter accepting a float as input (instead of int16).

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:86: *sample *
static_cast<float>(mic_level_) / static_cast<float>(
On 2017/06/29 05:45:27, peah-webrtc wrote:
> Are the static casts really needed? Since sample is of float, and the
conversion
> goes from left to right, that case should be automatic by the rules of C++ I
> think.

Done.

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:91: *sample *
static_cast<float>(mic_level_) / 255.0f);
On 2017/06/29 05:45:27, peah-webrtc wrote:
> The cast is not needed.

Done.

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:123: [this](float&
x) { worker_->ModifySampleFloat(&x); });
On 2017/06/29 05:45:27, peah-webrtc wrote:
> Why not pass the whole vector into ModifySampleFloat?

Just wanted to decouple FakeRecordingDeviceWorker implementations from a buffer
type (to avoid code duplication across the implementations).
However, the overhead of a lambda invoked for each sample is not that nice
either.

Thanks to your comments I realized that it's better to fully delegate
FakeRecordingDeviceWorker implementations, since a simulated device may have
memory and hence require a different way to iterate through the samples -
depending on the specific buffer type. For instance, AudioFrame has interleaved
samples and therefore care must be taken if we want to simulate soft-clipping
with memory.

WDYT?

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device.cc:134:
[this](int16_t& x) { worker_->ModifySampleInt16(&x); });
On 2017/06/29 05:45:27, peah-webrtc wrote:
> Why not just pass the whole vector into ModifySampleInt16? Even though this is
> not realtime code, it still makes sense to write it in a fast way if possible,
> and I don't see a big gain in terms of readability for making separate calls
on
> a sample basis.

Addressed (see previous comment).
Also good to correctly implement the iteration over channels.

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:29: // Abstract
class for simulating a microphone with analog gain.
On 2017/06/29 05:45:27, peah-webrtc wrote:
> This is not an abstract class, right? Or did I get it wrong?

Sorry, this comment applies to the previous PS, I forgot to update.

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

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/wav_based_simulator.cc:16: #include
<utility>
On 2017/06/29 05:45:27, peah-webrtc wrote:
> Why is utility needed?

Good catch, thanks!

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/wav_based_simulator.cc:19: #include
"webrtc/modules/audio_processing/test/fake_recording_device.h"
On 2017/06/29 05:45:27, peah-webrtc wrote:
> This include is not needed.

Done.

https://codereview.webrtc.org/2834643002/diff/220001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/wav_based_simulator.cc:61: :
AudioProcessingSimulator(settings) {}
On 2017/06/29 05:45:27, peah-webrtc wrote:
> Is this caused by git cl format? Was the indendation wrong before?

Right.

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/BUILD.gn (right):

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/BUILD.gn:602: "aec_dump:aec_dump_unittests",
Unrelated (merge)

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/BUILD.gn:764: "../../base:rtc_task_queue",
Unrelated (merge)

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/BUILD.gn:770: "aec_dump:aec_dump_impl",
Unrelated (merge)

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

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/aec_dump_based_simulator.cc:71: <<
"Simulating analog mic gain using AEC dump as input "
Just git cl format

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

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.cc:24: #include
"webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h"
Unrelated (merge)

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.cc:95:
worker_queue_("file_writer_task_queue") {
last line is unrelated (merge)

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.cc:296:
ap_->DetachAecDump();
Unrelated (merge)

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.cc:437:
*settings_.aec_dump_output_filename, -1, &worker_queue_));
Unrelated (merge)

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

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.h:22: #include
"webrtc/base/task_queue.h"
Unrelated (merge)

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/audio_processing_simulator.h:187:
rtc::TaskQueue worker_queue_;
Unrelated (merge)

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

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc:55:
rtc::MakeUnique<ChannelBuffer<float>>(data[0].size(), data.size());
Just git cl format

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc:63: const
ChannelBuffer<float>* curr) {
Just git cl format

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc:77: }
Just git cl format

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc:85: const
ChannelBuffer<float>* dst) {
Just git cl format

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc:100: }
Just git cl format

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc:134:
RTC_DCHECK_NE(buff->channels()[kC][kS], kTestMultiChannelSamples[kC][kS]);
Just git cl format

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc:188:
fake_rec_device_kind);
Just git cl format

https://codereview.webrtc.org/2834643002/diff/290001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/fake_recording_device_unittest.cc:225:
fake_rec_device_kind);
Just git cl format
Sign in to reply to this message.

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