Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2848593002:
AGC2 as a new APM sub-module operating with hard-coded gain. (Closed)
|
Created:
3 years, 7 months ago by AleBzk Modified:
3 years, 7 months 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. |
DescriptionThis CL introduces a new APM sub-module named AGC2 that does not use the band
split domain and only implements floating point operations (to avoid spectral
leakage issues and unnecessary complexity).
The goal of this CL is adding the new sub-module into APM without providing an
implementation that could replace the existing gain control modules. The focus
is in fact on initialization, reset, and configuration of AGC2.
The module itself only applies a hard-coded gain value. This behavior will
change in the coming CLs.
BUG=webrtc:7494
Review-Url: https://codereview.webrtc.org/2848593002
Cr-Commit-Position: refs/heads/master@{#18222}
Committed: https://chromium.googlesource.com/external/webrtc/+/3ec96df9070a0395c7d21d41c48bfe68b26cba92
Patch Set 1 #Patch Set 2 : AGC2 in APM with enabled flag #
Total comments: 30
Patch Set 3 : mock and iface removed, mv agc2 gain_controller2 #
Total comments: 10
Patch Set 4 : AGC2 Process() with hard coded gain, init and enable in APM #
Total comments: 22
Patch Set 5 : DigitalGainApplier and UTs, AGC2 before LC, minor changes #
Total comments: 4
Patch Set 6 : AudioBuffer only used at AGC2 API level, ArrayView<float> for internal impl #
Total comments: 14
Patch Set 7 : comments addressed + merge #Patch Set 8 : unneeded DCHECK removed #Messages
Total messages: 34 (14 generated)
Description was changed from ========== *** DO NOT LAND THIS CL *** Possible design of a new version of AGC. This CL is only meant to facilitate the design discussion. BUG= ========== to ========== *** DO NOT LAND THIS CL *** Possible design of a new version of AGC. This CL is only meant to facilitate the design discussion. BUG= ==========
alessiob@webrtc.org changed reviewers: + aleloi@webrtc.org
Description was changed from ========== *** DO NOT LAND THIS CL *** Possible design of a new version of AGC. This CL is only meant to facilitate the design discussion. BUG= ========== to ========== *** [DO NOT LAND THIS CL] *** Possible design of a new version of AGC. This CL is only meant to facilitate the design discussion. BUG= ==========
Hi Alex, As discussed offline, this CL is only a personal "experiment". Just wanted to ask you feedback at this step, even if the final goal is to have AGC2 substituting AGC (experimental) and suggesting a fixed gain. This will help me better understand how an APM sub-module fits in APM and how the indicated gain is used. I confess that some things are more copy-paste, for instance I didn't thoroughly checked if adding the AGC2 enable flag in AudioProcessingImpl::capture_nonlocked_ is a good idea. If you have time to take a look and you notice sth clearly incorrect, feel free to comment and suggest better ways to go. Alessio
Nice work! Suggestion: perhaps add a few proccessing / analyzing functions that do nothing, and adding calls to them in the ProcessStream methods to understand the calling/locking scheme. Also copy/adapt some integration tests to make these calls. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/BUILD.gn:113: "agc2/agc2_impl.h", I wonder how difficult it would be to write new submodules as separate build targets. Or maybe split the audio_processing target up as part of our work. Putting the build files in a subdirectory would simplify this file. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/BUILD.gn:505: "agc2/agc2_mock.h", Same for tests. New module unittests could have a separate target, on which this target depends. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:16: class Agc2 { I think we should strive towards more descriptive names when we write 'real' CLs for review. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:18: virtual ~Agc2() = default; I remember a recent virtual/not virtual discussion with @peah. I think having an empty virtual interface doesn't add much and could cost processing time. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2_impl.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2_impl.h:32: // - it does not work in the band split domain. A few points from peah's list are worth mentioning. 'Can operate at several modes in parallel' etc.
Description was changed from ========== *** [DO NOT LAND THIS CL] *** Possible design of a new version of AGC. This CL is only meant to facilitate the design discussion. BUG= ========== to ========== This CL introduces a new APM sub-module named AGC2 that does not use the band split domain. Initially, it will be used as AGG working in the fixed gain mode only. This allows to cascade AGC2 to the existing AGC. In the future, AGC2 will entirely replace the current AGC (outside the scope of this CL). BUG=webrtc:7494 ==========
peah@webrtc.org changed reviewers: + peah@webrtc.org
https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:16: class Agc2 { On 2017/05/02 10:11:53, aleloi wrote: > I think we should strive towards more descriptive names when we write 'real' CLs > for review. Agree, what about GainController2 ? https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:18: virtual ~Agc2() = default; On 2017/05/02 10:11:53, aleloi wrote: > I remember a recent virtual/not virtual discussion with @peah. I think having an > empty virtual interface doesn't add much and could cost processing time. > +1 on that. I don't think we should make this an interface if we don't absolutely need to. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2_impl.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2_impl.h:35: explicit Agc2Impl(rtc::CriticalSection* crit); If this API is not public, you don't need to bother with critical sections. I think we should only have locks inside APM and not in it's submodules. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2_mock.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2_mock.h:21: class Agc2Mock : Agc2 { I don't think we need a mock of AGC2. The main advantage that I see of that is that it would be possible to verify the integration of AGC within APM, but for the moment we are not able to do that anyway with the other submodules. Therefore I think it makes sense not to add it at this point. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:275: struct Agc2 { Along the lines of the comments from aleloi@, GainController2 and gain_controller2 would be more propriate names here.
Thanks for your feedback. Before uploading a new PS, I'd like that you answer my replies. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/BUILD.gn:113: "agc2/agc2_impl.h", On 2017/05/02 10:11:53, aleloi wrote: > I wonder how difficult it would be to write new submodules as separate build > targets. Or maybe split the audio_processing target up as part of our work. > Putting the build files in a subdirectory would simplify this file. Personally, I like your idea a lot. I'll do this change since we have everything in agc2/. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/BUILD.gn:505: "agc2/agc2_mock.h", On 2017/05/02 10:11:53, aleloi wrote: > Same for tests. New module unittests could have a separate target, on which this > target depends. Yup. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:16: class Agc2 { On 2017/05/02 10:11:53, aleloi wrote: > I think we should strive towards more descriptive names when we write 'real' CLs > for review. Any suggestion? I personally favor expanded names, so in this case I would have preferred AdaptiveGainControl. In this CL, I took the recent work on AEC as a reference. WDYT? https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:18: virtual ~Agc2() = default; On 2017/05/02 10:11:53, aleloi wrote: > I remember a recent virtual/not virtual discussion with @peah. I think having an > empty virtual interface doesn't add much and could cost processing time. > Well, I think it's safer to declare the dtor as virtual (see item 7 in Effective C++). I'll be happy to discuss this point offline. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:18: virtual ~Agc2() = default; On 2017/05/02 11:01:08, peah-webrtc wrote: > On 2017/05/02 10:11:53, aleloi wrote: > > I remember a recent virtual/not virtual discussion with @peah. I think having > an > > empty virtual interface doesn't add much and could cost processing time. > > > > +1 on that. I don't think we should make this an interface if we don't > absolutely need to. Having an interface allows mocking. Which otherwise becomes a pain. WDYT for this case? https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2_impl.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2_impl.h:32: // - it does not work in the band split domain. On 2017/05/02 10:11:53, aleloi wrote: > A few points from peah's list are worth mentioning. 'Can operate at several > modes in parallel' etc. I realized that maybe we want to document only what is implemented. Since the goal of this CL is having an AGC that only implements fixed gain and that can be used in combination with the existing AGC (to allow adaptive gain in cascade with fixed gain), we could adapt this whole comment accordingly. WDYT? https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2_mock.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2_mock.h:21: class Agc2Mock : Agc2 { On 2017/05/02 11:01:08, peah-webrtc wrote: > I don't think we need a mock of AGC2. The main advantage that I see of that is > that it would be possible to verify the integration of AGC within APM, but for > the moment we are not able to do that anyway with the other submodules. > Therefore I think it makes sense not to add it at this point. Right. Before removing AGC2 interface and its mock, a question. Do we expect AGC2 to have dependencies we might want to inject? If so, I'd keep the mock to easily test calls to the outer interface. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:275: struct Agc2 { On 2017/05/02 11:01:08, peah-webrtc wrote: > Along the lines of the comments from aleloi@, GainController2 and > gain_controller2 would be more propriate names here. Acknowledged.
https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:16: class Agc2 { On 2017/05/02 11:31:54, AleBzk wrote: > On 2017/05/02 10:11:53, aleloi wrote: > > I think we should strive towards more descriptive names when we write 'real' > CLs > > for review. > > Any suggestion? > > I personally favor expanded names, so in this case I would have preferred > AdaptiveGainControl. > In this CL, I took the recent work on AEC as a reference. WDYT? In AEC3, the folder is called AEC3, but the main class is called EchoCanceller3. Regarding the name AdaptiveGainControl it is good. But I'd personally prefer GainControl as 1) It is in my opinion more correct as it includes the fixed gain which is not adaptive. 2) It is shorter. WDYT? https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:18: virtual ~Agc2() = default; On 2017/05/02 11:31:54, AleBzk wrote: > On 2017/05/02 11:01:08, peah-webrtc wrote: > > On 2017/05/02 10:11:53, aleloi wrote: > > > I remember a recent virtual/not virtual discussion with @peah. I think > having > > an > > > empty virtual interface doesn't add much and could cost processing time. > > > > > > > +1 on that. I don't think we should make this an interface if we don't > > absolutely need to. > > Having an interface allows mocking. Which otherwise becomes a pain. > WDYT for this case? It makes sense to include a mock if it is needed. But from the work on AEC3, where mocks were added for the first layers, my feeling is that the value of a mock is very limited at this level. It would basically only verify the integration of the AGC2 in APM, and I'd personally then rather use a functional test for that. Let's dicuss this further offline. I'd prefer to add a mock once it https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:18: virtual ~Agc2() = default; On 2017/05/02 11:31:54, AleBzk wrote: > On 2017/05/02 10:11:53, aleloi wrote: > > I remember a recent virtual/not virtual discussion with @peah. I think having > an > > empty virtual interface doesn't add much and could cost processing time. > > > > Well, I think it's safer to declare the dtor as virtual (see item 7 in Effective > C++). > I'll be happy to discuss this point offline. I'm not that good at this, but if this class is not final, then I think you are right. But if we avoid making this an interface, I don't think the virtual part is needed. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2_impl.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2_impl.h:32: // - it does not work in the band split domain. On 2017/05/02 11:31:54, AleBzk wrote: > On 2017/05/02 10:11:53, aleloi wrote: > > A few points from peah's list are worth mentioning. 'Can operate at several > > modes in parallel' etc. > > I realized that maybe we want to document only what is implemented. Since the > goal of this CL is having an AGC that only implements fixed gain and that can be > used in combination with the existing AGC (to allow adaptive gain in cascade > with fixed gain), we could adapt this whole comment accordingly. WDYT? That makes sense. I don't think the comment needs the todo list though. Those parts should be clear from looking at the code. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2_mock.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2_mock.h:21: class Agc2Mock : Agc2 { On 2017/05/02 11:31:54, AleBzk wrote: > On 2017/05/02 11:01:08, peah-webrtc wrote: > > I don't think we need a mock of AGC2. The main advantage that I see of that is > > that it would be possible to verify the integration of AGC within APM, but for > > the moment we are not able to do that anyway with the other submodules. > > Therefore I think it makes sense not to add it at this point. > > Right. Before removing AGC2 interface and its mock, a question. > Do we expect AGC2 to have dependencies we might want to inject? If so, I'd keep > the mock to easily test calls to the outer interface. I don't think we should have any depencencies if we don't need to. The current AGC does not have that and I don't see any reason for adding that. Therefore I think it is better to start simple, and add a mock in the future if need arises.
All the comments address except for the following suggestion from Alex: "Perhaps add a few proccessing / analyzing functions that do nothing, and adding calls to them in the ProcessStream methods to understand the calling/locking scheme. Also copy/adapt some integration tests to make these calls." I will address this in my next patch set; I wanted to send out for review these first changes. Note that I removed the locks including EXCLUSIVE_LOCKS_REQUIRED(crit_capture_) when void InitializeGainController2() is defined in audio_processing_impl.h. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:16: class Agc2 { On 2017/05/02 11:45:20, peah-webrtc wrote: > On 2017/05/02 11:31:54, AleBzk wrote: > > On 2017/05/02 10:11:53, aleloi wrote: > > > I think we should strive towards more descriptive names when we write 'real' > > CLs > > > for review. > > > > Any suggestion? > > > > I personally favor expanded names, so in this case I would have preferred > > AdaptiveGainControl. > > In this CL, I took the recent work on AEC as a reference. WDYT? > > In AEC3, the folder is called AEC3, but the main class is called EchoCanceller3. > > Regarding the name AdaptiveGainControl it is good. But I'd personally prefer > GainControl as > 1) It is in my opinion more correct as it includes the fixed gain which is not > adaptive. > 2) It is shorter. > > WDYT? > Let's stick to AEC3 way, I used GainControl2. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2.h:18: virtual ~Agc2() = default; On 2017/05/02 11:45:20, peah-webrtc wrote: > On 2017/05/02 11:31:54, AleBzk wrote: > > On 2017/05/02 11:01:08, peah-webrtc wrote: > > > On 2017/05/02 10:11:53, aleloi wrote: > > > > I remember a recent virtual/not virtual discussion with @peah. I think > > having > > > an > > > > empty virtual interface doesn't add much and could cost processing time. > > > > > > > > > > +1 on that. I don't think we should make this an interface if we don't > > > absolutely need to. > > > > Having an interface allows mocking. Which otherwise becomes a pain. > > WDYT for this case? > > It makes sense to include a mock if it is needed. But from the work on AEC3, > where mocks were added for the first layers, my feeling is that the value of a > mock is very limited at this level. It would basically only verify the > integration of the AGC2 in APM, and I'd personally then rather use a functional > test for that. > > Let's dicuss this further offline. > > I'd prefer to add a mock once it From your comments, I understand that for the time being there's no need of any mock. So, I removed the interface and the mock. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2_impl.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2_impl.h:35: explicit Agc2Impl(rtc::CriticalSection* crit); On 2017/05/02 11:01:08, peah-webrtc wrote: > If this API is not public, you don't need to bother with critical sections. I > think we should only have locks inside APM and not in it's submodules. Acknowledged. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/agc2_mock.h (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/agc2_mock.h:21: class Agc2Mock : Agc2 { On 2017/05/02 11:45:20, peah-webrtc wrote: > On 2017/05/02 11:31:54, AleBzk wrote: > > On 2017/05/02 11:01:08, peah-webrtc wrote: > > > I don't think we need a mock of AGC2. The main advantage that I see of that > is > > > that it would be possible to verify the integration of AGC within APM, but > for > > > the moment we are not able to do that anyway with the other submodules. > > > Therefore I think it makes sense not to add it at this point. > > > > Right. Before removing AGC2 interface and its mock, a question. > > Do we expect AGC2 to have dependencies we might want to inject? If so, I'd > keep > > the mock to easily test calls to the outer interface. > > I don't think we should have any depencencies if we don't need to. The current > AGC does not have that and I don't see any reason for adding that. Therefore I > think it is better to start simple, and add a mock in the future if need arises. > Mock removed.
Nice! LGTM, conditioned that you have a look at the comments I added. https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/gain_controller2.h (right): https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:21: // Gain Controller 2 is an alternative gain control module that will entirely I'd prefer a comment which describes what the class is/does and does not refer to the current AGC. https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:680: << "gain_controller2: " What about also adding a LOG(LS_INFO) about that the gain controller is turned on, similarly for the other modules? https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:188: bool adaptive_gain_controller2_enabled_ = false; What about changing this to gain_controller2_enabled_, as that is shorter, and more correct? https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:271: // Gain Controller 2 enables the next generation AGC functionality. This Gain Controller 2 enables -> Enables https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:272: // feature replaces the standard methods of adaptive gain control in the adaptive gain control -> gain control
On 2017/05/05 20:44:11, peah-webrtc wrote: > Nice! > LGTM, conditioned that you have a look at the comments I added. > > https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/agc2/gain_controller2.h (right): > > https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/agc2/gain_controller2.h:21: // Gain Controller 2 > is an alternative gain control module that will entirely > I'd prefer a comment which describes what the class is/does and does not refer > to the current AGC. > > https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.cc (right): > > https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.cc:680: << > "gain_controller2: " > What about also adding a LOG(LS_INFO) about that the gain controller is turned > on, similarly for the other modules? > > https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/audio_processing_impl.h (right): > > https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/audio_processing_impl.h:188: bool > adaptive_gain_controller2_enabled_ = false; > What about changing this to gain_controller2_enabled_, as that is shorter, and > more correct? > > https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... > File webrtc/modules/audio_processing/include/audio_processing.h (right): > > https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/include/audio_processing.h:271: // Gain > Controller 2 enables the next generation AGC functionality. This > Gain Controller 2 enables -> Enables > > https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... > webrtc/modules/audio_processing/include/audio_processing.h:272: // feature > replaces the standard methods of adaptive gain control in the > adaptive gain control -> gain control Ah, sorry, I missed that you haven't actually added any fixed gain processing yet. not lgtm then, until that functionality is added, or at least some processing call is added.
Description was changed from ========== This CL introduces a new APM sub-module named AGC2 that does not use the band split domain. Initially, it will be used as AGG working in the fixed gain mode only. This allows to cascade AGC2 to the existing AGC. In the future, AGC2 will entirely replace the current AGC (outside the scope of this CL). BUG=webrtc:7494 ========== to ========== This CL introduces a new APM sub-module named AGC2 that does not use the band split domain. Initially, it will be used as AGG working in the fixed gain mode only. This allows to cascade AGC2 to the existing AGC. In the future, AGC2 will entirely replace the current AGC (outside the scope of this CL). BUG=webrtc:7494 ==========
Patchset #4 (id:60001) has been deleted
Description was changed from ========== This CL introduces a new APM sub-module named AGC2 that does not use the band split domain. Initially, it will be used as AGG working in the fixed gain mode only. This allows to cascade AGC2 to the existing AGC. In the future, AGC2 will entirely replace the current AGC (outside the scope of this CL). BUG=webrtc:7494 ========== to ========== This CL introduces a new APM sub-module named AGC2 that does not use the band split domain and only implements floating point operations. The goal of this CL is adding the new sub-module into APM without providing an implementation that could replace the existing gain control modules. The focus is in fact on initialization, reset, and configuration of AGC2. The module itself only applies a hard-coded gain value. This behavior will change in the coming CLs. BUG=webrtc:7494 ==========
Hi, I could finally come back to AGC2. As anticipated in the CL description, the main goal is adding a new APM sub-module, so take a closer look at the way I initialize, configure and reset AGC2 inside APM. I've done that by checking how it's done for AEC3 and LC. As for AGC2 itself, it's doing something extremely simple. It exploits the GainApplier of LC to apply a hard-coded gain. I've done this to check with audioproc_f that AGC2 is correctly enabled/disabled producing the expected output. Look fw to getting your comments! Alessio https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/gain_controller2.h (right): https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:21: // Gain Controller 2 is an alternative gain control module that will entirely On 2017/05/05 20:44:10, peah-webrtc wrote: > I'd prefer a comment which describes what the class is/does and does not refer > to the current AGC. Done. https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:680: << "gain_controller2: " On 2017/05/05 20:44:11, peah-webrtc wrote: > What about also adding a LOG(LS_INFO) about that the gain controller is turned > on, similarly for the other modules? Done. https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:188: bool adaptive_gain_controller2_enabled_ = false; On 2017/05/05 20:44:11, peah-webrtc wrote: > What about changing this to gain_controller2_enabled_, as that is shorter, and > more correct? Done. https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:271: // Gain Controller 2 enables the next generation AGC functionality. This On 2017/05/05 20:44:11, peah-webrtc wrote: > Gain Controller 2 enables -> Enables Done. https://codereview.webrtc.org/2848593002/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:272: // feature replaces the standard methods of adaptive gain control in the On 2017/05/05 20:44:11, peah-webrtc wrote: > adaptive gain control -> gain control Done. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:15: #include <string> unrelated, suggested by linter https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:715: if (capture_nonlocked_.intelligibility_enabled != unrelated style fix https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1326: } Is this position ok for now? WDYT? https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.h (left): https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.h:44: class AgcManagerDirect; Unused. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/test/audio_processing_simulator.cc (right): https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/test/audio_processing_simulator.cc:275: } For now, I didn't add any sanity check preventing to use AGC2 with the current AGC.
Hi, Thanks for the new patch! I have added some comments. PTAL! https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/gain_controller2.cc (right): https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.cc:32: ++instance_count_; This increement needs to be atomic as APM can be instantiated concurrently. What about new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))) ? https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/gain_controller2.h (right): https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:19: #include "webrtc/modules/audio_processing/level_controller/gain_applier.h" I don't think we should reuse level_controller code like this, even if it is a prototype. This is not part of the API surface of LevelController. If we would want to make it general, I'd rather prefer to move this class out of the level controller before starting to use it elsewhere. I'm not saying it should be made general though. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:27: // microphone gain and/or applying digital gain. It does not use the band split I don't think you should mention the band-split nor the issues with that in the code. Instead write that in the CL description. Same thing applies to the floating point interface. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:48: // TODO(...): Remove once a meaningful gain controller mode is implemented. You need to have a name for the todo. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:49: const float hard_coded_gain_; fixed_gain_ sounds better. WDYT? https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:50: Please remove duplicate empty line. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1326: } On 2017/05/16 12:38:43, AleBzk wrote: > Is this position ok for now? WDYT? Perhaps move it to before the level controller. That would make more sense for the fixed gain.
Description was changed from ========== This CL introduces a new APM sub-module named AGC2 that does not use the band split domain and only implements floating point operations. The goal of this CL is adding the new sub-module into APM without providing an implementation that could replace the existing gain control modules. The focus is in fact on initialization, reset, and configuration of AGC2. The module itself only applies a hard-coded gain value. This behavior will change in the coming CLs. BUG=webrtc:7494 ========== to ========== This CL introduces a new APM sub-module named AGC2 that does not use the band split domain and only implements floating point operations (to avoid spectral leakage issues and unnecessary complexity). The goal of this CL is adding the new sub-module into APM without providing an implementation that could replace the existing gain control modules. The focus is in fact on initialization, reset, and configuration of AGC2. The module itself only applies a hard-coded gain value. This behavior will change in the coming CLs. BUG=webrtc:7494 ==========
Description was changed from ========== This CL introduces a new APM sub-module named AGC2 that does not use the band split domain and only implements floating point operations (to avoid spectral leakage issues and unnecessary complexity). The goal of this CL is adding the new sub-module into APM without providing an implementation that could replace the existing gain control modules. The focus is in fact on initialization, reset, and configuration of AGC2. The module itself only applies a hard-coded gain value. This behavior will change in the coming CLs. BUG=webrtc:7494 ========== to ========== This CL introduces a new APM sub-module named AGC2 that does not use the band split domain and only implements floating point operations (to avoid spectral leakage issues and unnecessary complexity). The goal of this CL is adding the new sub-module into APM without providing an implementation that could replace the existing gain control modules. The focus is in fact on initialization, reset, and configuration of AGC2. The module itself only applies a hard-coded gain value. This behavior will change in the coming CLs. BUG=webrtc:7494 ==========
Hi, Comments addressed :) PTAL Alessio https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/gain_controller2.cc (right): https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.cc:32: ++instance_count_; On 2017/05/16 13:06:57, peah-webrtc wrote: > This increement needs to be atomic as APM can be instantiated concurrently. > What about > new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))) > ? Thanks, great to learn this. Then the same change should apply to level controller. Let me know if you want that I create another CL for that change. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/gain_controller2.h (right): https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:19: #include "webrtc/modules/audio_processing/level_controller/gain_applier.h" On 2017/05/16 13:06:58, peah-webrtc wrote: > I don't think we should reuse level_controller code like this, even if it is a > prototype. This is not part of the API surface of LevelController. If we would > want to make it general, I'd rather prefer to move this class out of the level > controller before starting to use it elsewhere. > I'm not saying it should be made general though. Thanks for this comment. I wrote a new class named DigitalGainApplier in the AGC2 folder. Since AGC2 has digital and analog gain, I think it's better to add the "Digital" prefix. The class is not as fancy as that in LC. When more features are needed, we will add them. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:27: // microphone gain and/or applying digital gain. It does not use the band split On 2017/05/16 13:06:57, peah-webrtc wrote: > I don't think you should mention the band-split nor the issues with that in the > code. Instead write that in the CL description. Same thing applies to the > floating point interface. Done. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:48: // TODO(...): Remove once a meaningful gain controller mode is implemented. On 2017/05/16 13:06:58, peah-webrtc wrote: > You need to have a name for the todo. Done. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:49: const float hard_coded_gain_; On 2017/05/16 13:06:57, peah-webrtc wrote: > fixed_gain_ sounds better. WDYT? I started with that naming, but I didn't want the reader thinking that the fixed digital gain is implemented (it's not). "Hard-coded" should convey more the correct interpretation. I also added _digital_ to further clarify. Once this CL is lgtm'ed, the next will replace the hard-coded gain mode with a first fixed digital gain implementation. If you think I'm picky, no worries; |fixed_gain_| is also fine :) https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:50: On 2017/05/16 13:06:58, peah-webrtc wrote: > Please remove duplicate empty line. Done. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/audio_processing_impl.cc (right): https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/audio_processing_impl.cc:1326: } On 2017/05/16 13:06:58, peah-webrtc wrote: > On 2017/05/16 12:38:43, AleBzk wrote: > > Is this position ok for now? WDYT? > > Perhaps move it to before the level controller. That would make more sense for > the fixed gain. Right! Thanks
Awesome! Looks great! My main comment is the use of AudioBuffer inside AGC2 but since you anyway will be changing that code in upcoming CLs give an lgtm anyway. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/gain_controller2.cc (right): https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.cc:32: ++instance_count_; On 2017/05/18 08:31:37, AleBzk wrote: > On 2017/05/16 13:06:57, peah-webrtc wrote: > > This increement needs to be atomic as APM can be instantiated concurrently. > > What about > > new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))) > > ? > > Thanks, great to learn this. > Then the same change should apply to level controller. > Let me know if you want that I create another CL for that change. Good point :-) I guess the reason for that not being a problem is that the level controller is only active on mobile devices, where only one instance is created. If you want to create a CL for that change that would be great! https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/agc2/gain_controller2.h (right): https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:19: #include "webrtc/modules/audio_processing/level_controller/gain_applier.h" On 2017/05/18 08:31:37, AleBzk wrote: > On 2017/05/16 13:06:58, peah-webrtc wrote: > > I don't think we should reuse level_controller code like this, even if it is a > > prototype. This is not part of the API surface of LevelController. If we would > > want to make it general, I'd rather prefer to move this class out of the level > > controller before starting to use it elsewhere. > > I'm not saying it should be made general though. > > Thanks for this comment. > I wrote a new class named DigitalGainApplier in the AGC2 folder. > Since AGC2 has digital and analog gain, I think it's better to add the "Digital" > prefix. > The class is not as fancy as that in LC. When more features are needed, we will > add them. That sounds good. https://codereview.webrtc.org/2848593002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/agc2/gain_controller2.h:49: const float hard_coded_gain_; On 2017/05/18 08:31:37, AleBzk wrote: > On 2017/05/16 13:06:57, peah-webrtc wrote: > > fixed_gain_ sounds better. WDYT? > > I started with that naming, but I didn't want the reader thinking that the fixed > digital gain is implemented (it's not). "Hard-coded" should convey more the > correct interpretation. > > I also added _digital_ to further clarify. > > Once this CL is lgtm'ed, the next will replace the hard-coded gain mode with a > first fixed digital gain implementation. > > If you think I'm picky, no worries; |fixed_gain_| is also fine :) Np :-) At this stage, anything is actually fine since this variable will anyway be removed in upcoming CLs. So go for that. https://codereview.webrtc.org/2848593002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/digital_gain_applier.h (right): https://codereview.webrtc.org/2848593002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/digital_gain_applier.h:24: void Process(float gain, AudioBuffer* audio); I think we should avoid having AudioBuffer as an input for more than the first layer of AGC2. The reason for that is that we'll need to change it in the not so far future, and if it is integrated deep into AGC2 that change will be more difficult. Instead let Process take arrayview objects as input. https://codereview.webrtc.org/2848593002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/gain_controller2.cc (right): https://codereview.webrtc.org/2848593002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.cc:41: digital_gain_applier_.Process(hard_coded_digital_gain_, audio); As commented on elsewhere. AudioBuffer is handy to use, but we'll need to change it in the future so I'd prefer to create ArrayViews from the AudioBuffer vectors and operate on those instead.
Thanks Per! @Alex: let me know if you have further comments, otherwise I will land this Alessio https://codereview.webrtc.org/2848593002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/digital_gain_applier.h (right): https://codereview.webrtc.org/2848593002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/digital_gain_applier.h:24: void Process(float gain, AudioBuffer* audio); On 2017/05/18 10:51:19, peah-webrtc wrote: > I think we should avoid having AudioBuffer as an input for more than the first > layer of AGC2. The reason for that is that we'll need to change it in the not so > far future, and if it is integrated deep into AGC2 that change will be more > difficult. > > Instead let Process take arrayview objects as input. Done. https://codereview.webrtc.org/2848593002/diff/100001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/gain_controller2.cc (right): https://codereview.webrtc.org/2848593002/diff/100001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.cc:41: digital_gain_applier_.Process(hard_coded_digital_gain_, audio); On 2017/05/18 10:51:19, peah-webrtc wrote: > As commented on elsewhere. AudioBuffer is handy to use, but we'll need to change > it in the future so I'd prefer to create ArrayViews from the AudioBuffer vectors > and operate on those instead. Done.
2 more comments. https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/gain_controller2.cc (right): https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.cc:45: RTC_DCHECK_LT(0, audio->num_channels()); Since you anyway store sample_rate_hz_, I'd suggest that you DCHECK on the rate being the same in the process call as when AGC2 was created. https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.cc:45: RTC_DCHECK_LT(0, audio->num_channels()); Is this DCHECK necessary? It will fail for 0 number of channels, but the current code actually works with that as well.
LG, see comments https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/BUILD.gn:113: "agc2/agc2_impl.h", On 2017/05/02 11:31:54, AleBzk wrote: > On 2017/05/02 10:11:53, aleloi wrote: > > I wonder how difficult it would be to write new submodules as separate build > > targets. Or maybe split the audio_processing target up as part of our work. > > Putting the build files in a subdirectory would simplify this file. > > Personally, I like your idea a lot. I'll do this change since we have everything > in agc2/. Did you decide to keep everythnig in ':audio_processing'? https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/digital_gain_applier.cc (right): https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/digital_gain_applier.cc:18: const float kMaxSampleValue = 32767.0f; const -> constexpr https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/digital_gain_applier.cc:26: if (gain == 1.f) { return; } Good! I missed something like this once. It led to https://codereview.webrtc.org/2659423002 https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/gain_controller2.h (right): https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.h:26: // Gain Controller 2 aims to automatically adjusting levels by acting on the adjusting -> adjust https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.h:35: void Process(AudioBuffer* audio); Do we want the GainController to depend on 'AudioBuffer'? It'll make it difficult to make it a separate GN target. AudioBuffer is a big interface in which lots of parts are not needed for this class. It also makes it less clear that this component operates outside the band-split domain. Can we change the AudioBuffer to a struct wrapping a 'float**', num_channels, num_samples? See FloatAudioFrame in audio_processing/include/aec_dump.h of this not-yet-landed CL: https://codereview.webrtc.org/2778783002/
https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/BUILD.gn:113: "agc2/agc2_impl.h", On 2017/05/18 15:01:17, aleloi wrote: > On 2017/05/02 11:31:54, AleBzk wrote: > > On 2017/05/02 10:11:53, aleloi wrote: > > > I wonder how difficult it would be to write new submodules as separate build > > > targets. Or maybe split the audio_processing target up as part of our work. > > > Putting the build files in a subdirectory would simplify this file. > > > > Personally, I like your idea a lot. I'll do this change since we have > everything > > in agc2/. > > Did you decide to keep everythnig in ':audio_processing'? I think the build target idea sounds great as well. But I'm also ok with keeping everything in audio_processing. https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/gain_controller2.h (right): https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.h:35: void Process(AudioBuffer* audio); On 2017/05/18 15:01:17, aleloi wrote: > Do we want the GainController to depend on 'AudioBuffer'? It'll make it > difficult to make it a separate GN target. AudioBuffer is a big interface in > which lots of parts are not needed for this class. It also makes it less clear > that this component operates outside the band-split domain. > > > Can we change the AudioBuffer to a struct wrapping a 'float**', num_channels, > num_samples? See FloatAudioFrame in audio_processing/include/aec_dump.h of this > not-yet-landed CL: https://codereview.webrtc.org/2778783002/ I also think that we should limit the AudioBuffer usage. Is there some way that we easily can avoid it being part of GainController2, without adding a lot of boilerplate code to audio_processing_impl.cc?
Thanks again! I address everything except for two points for which I will create two separate CLs: - adding a dedicated BUILD file - removing AudioBuffer dependency If you're all ok, I will land this CL. https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/BUILD.gn (right): https://codereview.webrtc.org/2848593002/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/BUILD.gn:113: "agc2/agc2_impl.h", On 2017/05/18 20:17:52, peah-webrtc wrote: > On 2017/05/18 15:01:17, aleloi wrote: > > On 2017/05/02 11:31:54, AleBzk wrote: > > > On 2017/05/02 10:11:53, aleloi wrote: > > > > I wonder how difficult it would be to write new submodules as separate > build > > > > targets. Or maybe split the audio_processing target up as part of our > work. > > > > Putting the build files in a subdirectory would simplify this file. > > > > > > Personally, I like your idea a lot. I'll do this change since we have > > everything > > > in agc2/. > > > > Did you decide to keep everythnig in ':audio_processing'? > > I think the build target idea sounds great as well. But I'm also ok with keeping > everything in audio_processing. Let's make another CL for this. https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/digital_gain_applier.cc (right): https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/digital_gain_applier.cc:18: const float kMaxSampleValue = 32767.0f; On 2017/05/18 15:01:17, aleloi wrote: > const -> constexpr Done. https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/digital_gain_applier.cc:26: if (gain == 1.f) { return; } On 2017/05/18 15:01:17, aleloi wrote: > Good! I missed something like this once. It led to > https://codereview.webrtc.org/2659423002 Acknowledged. https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/gain_controller2.cc (right): https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.cc:45: RTC_DCHECK_LT(0, audio->num_channels()); On 2017/05/18 12:27:05, peah-webrtc wrote: > Since you anyway store sample_rate_hz_, I'd suggest that you DCHECK on the rate > being the same in the process call as when AGC2 was created. I couldn't find any sample_rate property in AudioBuffer. I therefore added a DCHECK in APM against AudioProcessingImpl::proc_sample_rate_hz() (see audio_processing_impl.cc line 1321. https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/gain_controller2.h (right): https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.h:26: // Gain Controller 2 aims to automatically adjusting levels by acting on the On 2017/05/18 15:01:17, aleloi wrote: > adjusting -> adjust Done. https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.h:35: void Process(AudioBuffer* audio); On 2017/05/18 20:17:52, peah-webrtc wrote: > On 2017/05/18 15:01:17, aleloi wrote: > > Do we want the GainController to depend on 'AudioBuffer'? It'll make it > > difficult to make it a separate GN target. AudioBuffer is a big interface in > > which lots of parts are not needed for this class. It also makes it less clear > > that this component operates outside the band-split domain. > > > > > > Can we change the AudioBuffer to a struct wrapping a 'float**', num_channels, > > num_samples? See FloatAudioFrame in audio_processing/include/aec_dump.h of > this > > not-yet-landed CL: https://codereview.webrtc.org/2778783002/ > > I also think that we should limit the AudioBuffer usage. Is there some way that > we easily can avoid it being part of GainController2, without adding a lot of > boilerplate code to audio_processing_impl.cc? I used AudioBuffer for the first time recently and noticed that it is full of responsibilities (which I personally don't like). So, I was happy to hear that its usage will be deprecated at some point. However, I am not sure if we want our own "workaround" or, as Per said, boilerplate code in APM (when GainController2::Process() is called). Let's land this CL first and then make another one in which we remove the dependency on AudioBuffer - let's discuss which way to go offline.
https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/gain_controller2.cc (right): https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.cc:45: RTC_DCHECK_LT(0, audio->num_channels()); On 2017/05/19 13:15:41, AleBzk wrote: > On 2017/05/18 12:27:05, peah-webrtc wrote: > > Since you anyway store sample_rate_hz_, I'd suggest that you DCHECK on the > rate > > being the same in the process call as when AGC2 was created. > > I couldn't find any sample_rate property in AudioBuffer. > I therefore added a DCHECK in APM against > AudioProcessingImpl::proc_sample_rate_hz() (see audio_processing_impl.cc line > 1321. Ah, sorry, I forgot about it not being present. Please remove the check in line 1321 above. The problem is that you add an api call in AGC just for that DCHECK which make the API more complex and clutters the code. Furthermore, I think it can be discussed whether it really increases the code security. My aim with the DCHECK suggestion was that since you anyway had stored the sample rate, you basically got that DCHECK for free. https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/agc2/gain_controller2.h (right): https://codereview.webrtc.org/2848593002/diff/120001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/agc2/gain_controller2.h:35: void Process(AudioBuffer* audio); On 2017/05/19 13:15:41, AleBzk wrote: > On 2017/05/18 20:17:52, peah-webrtc wrote: > > On 2017/05/18 15:01:17, aleloi wrote: > > > Do we want the GainController to depend on 'AudioBuffer'? It'll make it > > > difficult to make it a separate GN target. AudioBuffer is a big interface in > > > which lots of parts are not needed for this class. It also makes it less > clear > > > that this component operates outside the band-split domain. > > > > > > > > > Can we change the AudioBuffer to a struct wrapping a 'float**', > num_channels, > > > num_samples? See FloatAudioFrame in audio_processing/include/aec_dump.h of > > this > > > not-yet-landed CL: https://codereview.webrtc.org/2778783002/ > > > > I also think that we should limit the AudioBuffer usage. Is there some way > that > > we easily can avoid it being part of GainController2, without adding a lot of > > boilerplate code to audio_processing_impl.cc? > > I used AudioBuffer for the first time recently and noticed that it is full of > responsibilities (which I personally don't like). > So, I was happy to hear that its usage will be deprecated at some point. > > However, I am not sure if we want our own "workaround" or, as Per said, > boilerplate code in APM (when GainController2::Process() is called). Let's land > this CL first and then make another one in which we remove the dependency on > AudioBuffer - let's discuss which way to go offline. That sounds good I think. Replacing AudioBuffer is a huge thing, and I'd rather for now settle for refraining from using it inside the submodules.
The CQ bit was checked by alessiob@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/2848593002/#ps160001 (title: "unneeded DCHECK removed")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495458441784770,
"parent_rev": "c0876aab46356cb4a6ead71b2605301dfce31619", "commit_rev":
"3ec96df9070a0395c7d21d41c48bfe68b26cba92"}
Message was sent while issue was closed.
Description was changed from ========== This CL introduces a new APM sub-module named AGC2 that does not use the band split domain and only implements floating point operations (to avoid spectral leakage issues and unnecessary complexity). The goal of this CL is adding the new sub-module into APM without providing an implementation that could replace the existing gain control modules. The focus is in fact on initialization, reset, and configuration of AGC2. The module itself only applies a hard-coded gain value. This behavior will change in the coming CLs. BUG=webrtc:7494 ========== to ========== This CL introduces a new APM sub-module named AGC2 that does not use the band split domain and only implements floating point operations (to avoid spectral leakage issues and unnecessary complexity). The goal of this CL is adding the new sub-module into APM without providing an implementation that could replace the existing gain control modules. The focus is in fact on initialization, reset, and configuration of AGC2. The module itself only applies a hard-coded gain value. This behavior will change in the coming CLs. BUG=webrtc:7494 Review-Url: https://codereview.webrtc.org/2848593002 Cr-Commit-Position: refs/heads/master@{#18222} Committed: https://chromium.googlesource.com/external/webrtc/+/3ec96df9070a0395c7d21d41c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/3ec96df9070a0395c7d21d41c... |
