Add new ANA stats to GetStats() to count the number of actions taken by each controller.

webrtc/api/audio_codecs/audio_encoder.h

Index: webrtc/api/audio_codecs/audio_encoder.h
diff --git a/webrtc/api/audio_codecs/audio_encoder.h b/webrtc/api/audio_codecs/audio_encoder.h
index bd8c7deb942ab9bf9006c3845c7bd9d996291c4b..6fdb4995f5a65917576a48721268c9c50864e77f 100644
--- a/webrtc/api/audio_codecs/audio_encoder.h
+++ b/webrtc/api/audio_codecs/audio_encoder.h
@@ -80,6 +80,18 @@ class AudioEncoder {
     std::vector<EncodedInfoLeaf> redundant;
   };
 
+  // Statistics that can be obtained from an AudioEncoder.
+  struct AudioEncoderStats {
+    AudioEncoderStats();
+    AudioEncoderStats(const AudioEncoderStats&);
+    ~AudioEncoderStats();
+    rtc::Optional<int> ana_bitrate_action_counter;

[ossu, 2017/08/31 13:13:10]

Are all these individually optional? 

Since they're counters, it would seem to me they're not optional at all: is a
missing value different from a count of zero? Otherwise they should all just be
ints.

[ivoc, 2017/08/31 14:57:50]

I think some Optionality is needed here, since ANA can be completely disabled,
and in that case we don't want to report these stats. We want to be able to tell
a difference between calls with 0 actions (this means good enough network
conditions that ANA is not needed), and calls where ANA was completely disabled.
However, I guess it's possible to put all of the int's into a struct, and make
that struct Optional. Do you think that would be better?

[ossu, 2017/08/31 15:40:20]

1. If ANA being enabled or not is one statistic we want to communicate, then
maybe that's just a separate bool field?
2. If AudioEncoderStats is going to be either "empty" (ANA disabled) or full of
values (ANA enabled), then I think the functions returning AudioEncoderStats
should return rtc::Optional<AudioEncoderStats>.

Both of these depend on the future direction of this statistics struct. Is it
planned to, in future, contain other stats that aren't ANA specific? If not,
perhaps rename it to ANAStats (or something like that). Then having something
return an Optional<ANAStats> makes it quite clear that if there's no ANA
enabled, there'll be no stats for it. If it's conceivable that AudioEncoderStats
will at some point contain other information, but there's nothing planned right
now, then going the Optional<ANAStats> route also makes sense; Once more stats
are needed, the Optional<ANAStats> can be put inside the , then new,
AudioEncoderStats struct. 

[ivoc, 2017/09/01 09:34:24]

Re 1: I think it would be a bit strange to have a stat in GetStats() (which is
for monitoring fluctuating stats) that is either true or false throughout the
call, just to indicate if a module is active or not. To me it makes more sense
to have the stats in GetStats() when ANA is active, and not set them when ANA is
not used.

About your other proposal, I think you have a good point, indeed my reasoning
was that in the future we may want to add more stats to AudioEncoder. I like
your proposal, but it does mean that if we want to add other stats to
AudioEncoder we will have to change the interface of GetStats() function, which
is a public API (or will be). WDYT?

I'm also not sure how likely it is that we will want to add other non-ANA
related stats from AudioEncoder to GetStats(), Minyue, what do you think?

[ossu, 2017/09/01 14:28:52]

I guess if we don't really know if other stats will be exposed from the
AudioEncoder object right now, the most descriptive way forward would be:
  rtc::Optional<ANAStats> GetANAStats() const;
or some very similar naming scheme. Then if there's a future with more stats,
that could be implemented as a more general GetStats() that returns something
that could contain an rtc::Optional<ANAStats> as one of its members (and
GetANAStats could become deprecated and eventually removed).

Since any downstream code that wants to use the new stats would need to be
updated anyways, this shouldn't be more breaking than changing an existing
GetStats() signature or adding members to an existing struct.

[ivoc, 2017/09/01 15:27:16]

Thanks, that sounds like a good plan. As discussed offline, I kept the
per-controller optionality since it seems that ANA is pretty flexible with
sub-modules that can be individually enabled/disabled.

[hbos, 2017/09/04 08:49:54]

Using optional is correct. If the value is being counted it should start at 0,
but if it's N/A (such as not enabled for counting) then it shouldn't be present.

Doing Optional<ANAStats> makes sense if its all-or-nothing. Having ANAStats with
Optional fields also makes sense.

+    rtc::Optional<int> ana_channel_action_counter;
+    rtc::Optional<int> ana_dtx_action_counter;
+    rtc::Optional<int> ana_fec_action_counter;
+    rtc::Optional<int> ana_frame_length_action_counter;
+  };
+
   virtual ~AudioEncoder() = default;
 
   // Returns the input sample rate in Hz and the number of input channels.
@@ -203,6 +215,9 @@ class AudioEncoder {
   virtual void SetReceiverFrameLengthRange(int min_frame_length_ms,
                                            int max_frame_length_ms);
 
+  // Get statistics related to the audio encoder.
+  virtual AudioEncoderStats GetStats() const;
+
  protected:
   // Subclasses implement this to perform the actual encoding. Called by
   // Encode().