Moved the GainControlForNewAGC class to a separate file.

webrtc/modules/audio_processing/gain_control_for_new_agc.h

Index: webrtc/modules/audio_processing/gain_control_for_new_agc.h
diff --git a/webrtc/modules/audio_processing/gain_control_for_new_agc.h b/webrtc/modules/audio_processing/gain_control_for_new_agc.h
new file mode 100644
index 0000000000000000000000000000000000000000..5e8f8aceb5cf876982854e5e159a79b91a274999
--- /dev/null
+++ b/webrtc/modules/audio_processing/gain_control_for_new_agc.h
@@ -0,0 +1,66 @@
+/*
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#ifndef WEBRTC_MODULES_AUDIO_PROCESSING_GAIN_CONTROL_FOR_NEW_AGC_H_
+#define WEBRTC_MODULES_AUDIO_PROCESSING_GAIN_CONTROL_FOR_NEW_AGC_H_
+
+#include "webrtc/base/constructormagic.h"
+#include "webrtc/modules/audio_processing/agc/agc_manager_direct.h"
+#include "webrtc/modules/audio_processing/gain_control_impl.h"

[the sun, 2016/02/08 09:21:39]

include not needed?

[peah-webrtc, 2016/02/08 13:01:12]

It is needed for GainControlImpl, right? Or do you prefer that a forward should
be used instead?

[the sun, 2016/02/08 14:18:11]

Yes, forward declare GainControl. To me it looks like you don't need to know
about GainControlImpl here (this class only uses the GainControl interface). 

[peah-webrtc, 2016/02/09 09:11:15]

Done.

+#include "webrtc/modules/audio_processing/include/audio_processing.h"
+
+namespace webrtc {
+
+// This class has two main functionalities:

[the sun, 2016/02/08 09:21:39]

functinoalities -> purposes

[peah-webrtc, 2016/02/08 13:01:12]

Done.

+//
+// 1) It is returned instead of the real GainControl after the new AGC has been
+//    enabled in order to prevent an outside user from overriding compression
+//    settings. It doesn't do anything in its implementation, except for
+//    delegating the const methods and Enable calls to the real GainControl, so
+//    AGC can still be disabled.
+//
+// 2) It is injected into AgcManagerDirect and implements volume callbacks for
+//    getting and setting the volume level. It just caches this value to be used
+//    in VoiceEngine later.
+class GainControlForNewAgc : public GainControl, public VolumeCallbacks {

[the sun, 2016/02/08 09:21:39]

Names with "new" and "old" in them will soon be obsolete and confusing. While
we're messing about, can we come up with something better? "Agc2"? Something
else that is more descriptive of this "new agc"?

[peah-webrtc, 2016/02/08 13:01:12]

Good point. I exchanged it with "Experimental" AGC as that matches what is
written elsewhere in the code. Although I don't think this is either a good
name, until I know more about the differences between the AGCs I think it could
be good tradeoff. Changing to a more proper name would in my mind be a separate
CL.

Does that make sense?

[the sun, 2016/02/08 14:18:11]

Yes, thanks!

[peah-webrtc, 2016/02/09 09:11:15]

Acknowledged.

+ public:
+  explicit GainControlForNewAgc(GainControlImpl* gain_control);
+
+  // GainControl implementation.
+  int Enable(bool enable) override;
+  bool is_enabled() const override;
+  int set_stream_analog_level(int level) override;
+  int stream_analog_level() override;
+  int set_mode(Mode mode) override;
+  Mode mode() const override;
+  int set_target_level_dbfs(int level) override;
+  int target_level_dbfs() const override;
+  int set_compression_gain_db(int gain) override;
+  int compression_gain_db() const override;
+  int enable_limiter(bool enable) override;
+  bool is_limiter_enabled() const override;
+  int set_analog_level_limits(int minimum, int maximum) override;
+  int analog_level_minimum() const override;
+  int analog_level_maximum() const override;
+  bool stream_is_saturated() const override;
+
+  void SetMicVolume(int volume) override;

[the sun, 2016/02/08 09:21:39]

// VolumeCallbacks implementation.
?

[peah-webrtc, 2016/02/08 13:01:12]

Done.

+  int GetMicVolume() override;
+
+ private:
+  GainControl* real_gain_control_;

[the sun, 2016/02/08 09:21:39]

GainControlImpl?
and you can forward declare it.

[peah-webrtc, 2016/02/08 13:01:12]

This is a bit tricky. As it is currently implemented some GainControl methods
referenced in the class (e.g., Enable()) are private in GainControlImpl but
public in GainControl. That means that if the code is changed to use 
GainControlImpl instead of GainControl, it won't be able to access those
methods.

I don't know the reason for having a more restricted access in GainControlImpl
compared to GainControl though. I could change that in this CL if you think it
is motivated.

What do you think?

[the sun, 2016/02/08 14:18:11]

Typically the reason for making a virtual method private in a derived class is
to prevent further overriding (i.e. make the class "final" in the Java sense).

If you give this class a GainControl*, the private override in GainControlImpl
will still be called if you've given GainControlForExperimentalAgc a pointer to
a GainControlImpl. It is the access rights declared in GainControl that matters
if you use the object as one of those, and I think you can.

[peah-webrtc, 2016/02/09 09:11:15]

Maybe I got this wrong, but the problem I see is that if I change this to an
GainControlImpl*, I cannot use the private methods in GainControlImpl (e.g.,
Enable) from within the GainControlForNewAgc class. But if I keep it as a
GainControl* the methods I need to use are public (e.g., Enable) which means I
can use them.

One way to circumvent that would of course be to store the pointer as
GainControlImpl* and cast to GainControl* each time I want to access these
methods. But that seems not like a good solution.

I'd rather instead change the constructor receive a GainControl* instead of a
GainControlImpl*. I'll do that change. Please let me know what you think about
it!

[the sun, 2016/02/09 11:49:55]

No, sorry, you got it right; I was just bad at expressing myself.

+  int volume_;
+
+  RTC_DISALLOW_COPY_AND_ASSIGN(GainControlForNewAgc);
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_AUDIO_PROCESSING_GAIN_CONTROL_FOR_NEW_AGC_H_