Issue 1299143003: Remove AgcManager.

Issue 1299143003: Remove AgcManager. (Closed)

Created:
5 years, 4 months ago by aluebs-webrtc

Modified:
5 years, 2 months ago

Reviewers:
Andrew MacDonald

CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), Andrew MacDonald, qiang.lu, tterriberry_mozilla.com, hlundin-webrtc, niklas.enbom, kwiberg-webrtc, bjornv1

Base URL:
https://chromium.googlesource.com/external/webrtc.git@master

Target Ref:
refs/pending/heads/master

Project:
webrtc

Visibility:
Public.

More Reviews

Description

Remove AgcManager. It was not used anywhere. R=andrew@webrtc.org Committed: https://crrev.com/d094c04baffabfa914682050615b97faea939bd1 Cr-Commit-Position: refs/heads/master@{#10113}

Patch Set 2 : Add AgcManagerDirectTest #

Total comments: 4

Patch Set 4 : Add initial volume test #

Patch Set 6 : Initialize volume #

Created: 5 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+705 lines, -1559 lines)			Patch
M	webrtc/modules/audio_processing/agc/agc_manager_direct.h	View	1 2	5 chunks	+16 lines, -12 lines	0 comments	Download
A	webrtc/modules/audio_processing/agc/agc_manager_direct_unittest.cc	View	1 2 3 4 5	1 chunk	+686 lines, -0 lines	0 comments	Download
M	webrtc/modules/modules.gyp	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
D	webrtc/test/fake_common.h	View	1	1 chunk	+0 lines, -44 lines	0 comments	Download
M	webrtc/tools/agc/agc_harness.cc	View	1 2	1 chunk	+0 lines, -1 line	0 comments	Download
D	webrtc/tools/agc/agc_manager.h	View		1 chunk	+0 lines, -81 lines	0 comments	Download
D	webrtc/tools/agc/agc_manager.cc	View	1 2	1 chunk	+0 lines, -255 lines	0 comments	Download
D	webrtc/tools/agc/agc_manager_integrationtest.cc	View		1 chunk	+0 lines, -123 lines	0 comments	Download
D	webrtc/tools/agc/agc_manager_unittest.cc	View	1 2	1 chunk	+0 lines, -741 lines	0 comments	Download
D	webrtc/tools/agc/agc_test.cc	View		1 chunk	+0 lines, -155 lines	0 comments	Download
M	webrtc/tools/tools.gyp	View		2 chunks	+2 lines, -31 lines	0 comments	Download
D	webrtc/voice_engine/mock/fake_voe_external_media.h	View	1 2	1 chunk	+0 lines, -69 lines	0 comments	Download
D	webrtc/voice_engine/mock/mock_voe_volume_control.h	View	1	1 chunk	+0 lines, -43 lines	0 comments	Download
M	webrtc/webrtc_tests.gypi	View	1 2	4 chunks	+0 lines, -4 lines	0 comments	Download

Messages

Total messages: 14 (1 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Andrew MacDonald

Everything looks good except for the test comment. https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processing/agc/agc_manager_direct.h File webrtc/modules/audio_processing/agc/agc_manager_direct.h (right): https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processing/agc/agc_manager_direct.h#newcode26 webrtc/modules/audio_processing/agc/agc_manager_direct.h:26: class ...

5 years, 4 months ago (2015-08-19 17:58:40 UTC) #3

Andrew MacDonald

Oh, and I would say "Remove" rather than "Deprecate" in the CL description.

5 years, 4 months ago (2015-08-19 17:59:07 UTC) #4

aluebs-webrtc

Changed the description and added AgcManagerDirectTest based on the AgcManagerUnittest one. https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processing/agc/agc_manager_direct.h File webrtc/modules/audio_processing/agc/agc_manager_direct.h (right): ...

5 years, 2 months ago (2015-09-28 22:51:08 UTC) #5

Andrew MacDonald

lgtm. Thanks for doing this. I'm sure it was pretty tiresome to convert that test. ...

5 years, 2 months ago (2015-09-29 05:33:34 UTC) #6

lgtm.

Thanks for doing this. I'm sure it was pretty tiresome to convert that test.

https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/agc/agc_manager_direct.h (right):

https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/agc/agc_manager_direct.h:26: class
VolumeCallbacks {
On 2015/09/28 22:51:08, aluebs-webrtc wrote:
> On 2015/08/19 17:58:39, Andrew MacDonald wrote:
> > Not for this CL, but do we need to use this callback approach any longer?
> > Perhaps add a TODO to remove if not.
> 
> Added TODO. But how would you imagine it to work?

Instead of triggering a callback on volume change, we'd have Process() indicate
it was ready to deliver a new volume. Similar to how the localizers work.

https://codereview.webrtc.org/1299143003/diff/1/webrtc/tools/agc/agc_manager_...
File webrtc/tools/agc/agc_manager_unittest.cc (left):

https://codereview.webrtc.org/1299143003/diff/1/webrtc/tools/agc/agc_manager_...
webrtc/tools/agc/agc_manager_unittest.cc:1: /*
On 2015/09/28 22:51:08, aluebs-webrtc wrote:
> On 2015/08/19 17:58:39, Andrew MacDonald wrote:
> > Unfortunately, I think we need to port this test to use AgcManagerDirect. I
> > think it was the main reason we were leaving AgcManager kicking around:
> because
> > otherwise AMD was insufficiently tested.
> 
> Added AgcManagerDirectTest based on this one. Unfortunately it doesn't diff
> against this one :(

It's OK, I downloaded the patch and diffed locally.

https://codereview.webrtc.org/1299143003/diff/20001/webrtc/tools/agc/agc_mana...
File webrtc/tools/agc/agc_manager_unittest.cc (left):

https://codereview.webrtc.org/1299143003/diff/20001/webrtc/tools/agc/agc_mana...
webrtc/tools/agc/agc_manager_unittest.cc:389: TEST_F(AgcManagerUnitTest,
LowInitialVolumeIsRaised) {
This test was removed. Chromium configures this itself now, right?

aluebs-webrtc

https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processing/agc/agc_manager_direct.h File webrtc/modules/audio_processing/agc/agc_manager_direct.h (right): https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processing/agc/agc_manager_direct.h#newcode26 webrtc/modules/audio_processing/agc/agc_manager_direct.h:26: class VolumeCallbacks { On 2015/09/29 05:33:34, Andrew MacDonald wrote: ...

5 years, 2 months ago (2015-09-29 18:10:40 UTC) #7

https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/agc/agc_manager_direct.h (right):

https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/agc/agc_manager_direct.h:26: class
VolumeCallbacks {
On 2015/09/29 05:33:34, Andrew MacDonald wrote:
> On 2015/09/28 22:51:08, aluebs-webrtc wrote:
> > On 2015/08/19 17:58:39, Andrew MacDonald wrote:
> > > Not for this CL, but do we need to use this callback approach any longer?
> > > Perhaps add a TODO to remove if not.
> > 
> > Added TODO. But how would you imagine it to work?
> 
> Instead of triggering a callback on volume change, we'd have Process()
indicate
> it was ready to deliver a new volume. Similar to how the localizers work.

And let the APM update the volume in the GainControlForNewAgc? Or are you
thinking of getting rid of this all? Then how would the VoE get notified about
the volume control? And how do we do this without disrupting how it works in
mobile, where the new AGC is not possible to use? We can take this offline when
the time comes :)

https://codereview.webrtc.org/1299143003/diff/1/webrtc/tools/agc/agc_manager_...
File webrtc/tools/agc/agc_manager_unittest.cc (left):

https://codereview.webrtc.org/1299143003/diff/1/webrtc/tools/agc/agc_manager_...
webrtc/tools/agc/agc_manager_unittest.cc:1: /*
On 2015/09/29 05:33:34, Andrew MacDonald wrote:
> On 2015/09/28 22:51:08, aluebs-webrtc wrote:
> > On 2015/08/19 17:58:39, Andrew MacDonald wrote:
> > > Unfortunately, I think we need to port this test to use AgcManagerDirect.
I
> > > think it was the main reason we were leaving AgcManager kicking around:
> > because
> > > otherwise AMD was insufficiently tested.
> > 
> > Added AgcManagerDirectTest based on this one. Unfortunately it doesn't diff
> > against this one :(
> 
> It's OK, I downloaded the patch and diffed locally.

:)

https://codereview.webrtc.org/1299143003/diff/20001/webrtc/tools/agc/agc_mana...
File webrtc/tools/agc/agc_manager_unittest.cc (left):

https://codereview.webrtc.org/1299143003/diff/20001/webrtc/tools/agc/agc_mana...
webrtc/tools/agc/agc_manager_unittest.cc:389: TEST_F(AgcManagerUnitTest,
LowInitialVolumeIsRaised) {
On 2015/09/29 05:33:34, Andrew MacDonald wrote:
> This test was removed. Chromium configures this itself now, right?

Exactly, the startup_min_volume is set in audio_processing.h in the Config
depending if Chromium or Native and passed in to the AgcManagerDirect, so no way
of testing it in the unittest. If you think I should add this test somewhere
else, I am happy to do so.

Andrew MacDonald

5 years, 2 months ago (2015-09-29 18:17:12 UTC) #8

https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/agc/agc_manager_direct.h (right):

https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/agc/agc_manager_direct.h:26: class
VolumeCallbacks {
On 2015/09/29 18:10:39, aluebs-webrtc wrote:
> On 2015/09/29 05:33:34, Andrew MacDonald wrote:
> > On 2015/09/28 22:51:08, aluebs-webrtc wrote:
> > > On 2015/08/19 17:58:39, Andrew MacDonald wrote:
> > > > Not for this CL, but do we need to use this callback approach any
longer?
> > > > Perhaps add a TODO to remove if not.
> > > 
> > > Added TODO. But how would you imagine it to work?
> > 
> > Instead of triggering a callback on volume change, we'd have Process()
> indicate
> > it was ready to deliver a new volume. Similar to how the localizers work.
> 
> And let the APM update the volume in the GainControlForNewAgc? Or are you
> thinking of getting rid of this all? Then how would the VoE get notified about
> the volume control? And how do we do this without disrupting how it works in
> mobile, where the new AGC is not possible to use? We can take this offline
when
> the time comes :)

These callbacks don't get delivered to voice engine. It requests the updated
value from APM:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

Same with Chrome:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

Hence the "direct" part of AgcManagerDirect.

https://codereview.webrtc.org/1299143003/diff/20001/webrtc/tools/agc/agc_mana...
File webrtc/tools/agc/agc_manager_unittest.cc (left):

https://codereview.webrtc.org/1299143003/diff/20001/webrtc/tools/agc/agc_mana...
webrtc/tools/agc/agc_manager_unittest.cc:389: TEST_F(AgcManagerUnitTest,
LowInitialVolumeIsRaised) {
On 2015/09/29 18:10:39, aluebs-webrtc wrote:
> On 2015/09/29 05:33:34, Andrew MacDonald wrote:
> > This test was removed. Chromium configures this itself now, right?
> 
> Exactly, the startup_min_volume is set in audio_processing.h in the Config
> depending if Chromium or Native and passed in to the AgcManagerDirect, so no
way
> of testing it in the unittest. If you think I should add this test somewhere
> else, I am happy to do so.

Do you want to add a quick test where you configure startup_min_volume and
verify that it's respected?

aluebs-webrtc

5 years, 2 months ago (2015-09-29 19:10:34 UTC) #9

https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/agc/agc_manager_direct.h (right):

https://codereview.webrtc.org/1299143003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/agc/agc_manager_direct.h:26: class
VolumeCallbacks {
On 2015/09/29 18:17:11, Andrew MacDonald wrote:
> On 2015/09/29 18:10:39, aluebs-webrtc wrote:
> > On 2015/09/29 05:33:34, Andrew MacDonald wrote:
> > > On 2015/09/28 22:51:08, aluebs-webrtc wrote:
> > > > On 2015/08/19 17:58:39, Andrew MacDonald wrote:
> > > > > Not for this CL, but do we need to use this callback approach any
> longer?
> > > > > Perhaps add a TODO to remove if not.
> > > > 
> > > > Added TODO. But how would you imagine it to work?
> > > 
> > > Instead of triggering a callback on volume change, we'd have Process()
> > indicate
> > > it was ready to deliver a new volume. Similar to how the localizers work.
> > 
> > And let the APM update the volume in the GainControlForNewAgc? Or are you
> > thinking of getting rid of this all? Then how would the VoE get notified
about
> > the volume control? And how do we do this without disrupting how it works in
> > mobile, where the new AGC is not possible to use? We can take this offline
> when
> > the time comes :)
> 
> These callbacks don't get delivered to voice engine. It requests the updated
> value from APM:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> Same with Chrome:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> 
> Hence the "direct" part of AgcManagerDirect.

Ok, I think I get your point but we can discuss when we move forward with this.

https://codereview.webrtc.org/1299143003/diff/20001/webrtc/tools/agc/agc_mana...
File webrtc/tools/agc/agc_manager_unittest.cc (left):

https://codereview.webrtc.org/1299143003/diff/20001/webrtc/tools/agc/agc_mana...
webrtc/tools/agc/agc_manager_unittest.cc:389: TEST_F(AgcManagerUnitTest,
LowInitialVolumeIsRaised) {
On 2015/09/29 18:17:11, Andrew MacDonald wrote:
> On 2015/09/29 18:10:39, aluebs-webrtc wrote:
> > On 2015/09/29 05:33:34, Andrew MacDonald wrote:
> > > This test was removed. Chromium configures this itself now, right?
> > 
> > Exactly, the startup_min_volume is set in audio_processing.h in the Config
> > depending if Chromium or Native and passed in to the AgcManagerDirect, so no
> way
> > of testing it in the unittest. If you think I should add this test somewhere
> > else, I am happy to do so.
> 
> Do you want to add a quick test where you configure startup_min_volume and
> verify that it's respected?

Added a small test that verifies that the MicVolume is set to kInitialVolume.

aluebs-webrtc

Committed patchset #6 (id:100001) manually as d094c04baffabfa914682050615b97faea939bd1 (presubmit successful).

5 years, 2 months ago (2015-09-29 22:45:32 UTC) #10

commit-bot: I haz the power

Patchset 6 (id:??) landed as https://crrev.com/d094c04baffabfa914682050615b97faea939bd1 Cr-Commit-Position: refs/heads/master@{#10113}

5 years, 2 months ago (2015-09-29 22:45:36 UTC) #11

Andrew MacDonald

https://codereview.webrtc.org/1299143003/diff/80001/webrtc/modules/audio_processing/agc/agc_manager_direct_unittest.cc File webrtc/modules/audio_processing/agc/agc_manager_direct_unittest.cc (right): https://codereview.webrtc.org/1299143003/diff/80001/webrtc/modules/audio_processing/agc/agc_manager_direct_unittest.cc#newcode102 webrtc/modules/audio_processing/agc/agc_manager_direct_unittest.cc:102: EXPECT_EQ(kInitialVolume, volume_.GetMicVolume()); Is this checking anything? The earlier test ...

5 years, 2 months ago (2015-09-29 22:49:32 UTC) #12

aluebs-webrtc

5 years, 2 months ago (2015-09-29 23:00:18 UTC) #13

Andrew MacDonald

5 years, 2 months ago (2015-09-29 23:02:26 UTC) #14

Message was sent while issue was closed.

https://codereview.webrtc.org/1299143003/diff/80001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/agc/agc_manager_direct_unittest.cc (right):

https://codereview.webrtc.org/1299143003/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc/agc_manager_direct_unittest.cc:102:
EXPECT_EQ(kInitialVolume, volume_.GetMicVolume());
On 2015/09/29 23:00:17, aluebs-webrtc wrote:
> On 2015/09/29 22:49:32, Andrew MacDonald wrote:
> > Is this checking anything? The earlier test sets the initial volume very low
> and
> > verifies that AGC returns the min allowable.
> > 
> > I was also thinking of the same thing but with the min volume configured
> first.
> 
> This one does the same. The volume_ is initialized with 0 volume and here it
> verifies it that it is bumped up by the AGC.

Got it, thanks.

https://codereview.webrtc.org/1299143003/diff/80001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/agc/agc_manager_direct_unittest.cc:106:
FirstProcess();
On 2015/09/29 23:00:18, aluebs-webrtc wrote:
> On 2015/09/29 22:49:32, Andrew MacDonald wrote:
> > Just curious, why did you switch these?
> 
> 2 additional of these were necessary to not break, the rest I added just to
> start in a consistent state in all tests. If you think it is better how it was
> before, I am happy to change them back.

No, this is fine.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages