Audio codec usage statistics

webrtc/modules/audio_coding/codecs/audio_encoder.h

Index: webrtc/modules/audio_coding/codecs/audio_encoder.h
diff --git a/webrtc/modules/audio_coding/codecs/audio_encoder.h b/webrtc/modules/audio_coding/codecs/audio_encoder.h
index a0f1839e73393c4ad017c6d6d5393a740187f64d..058c60fe084cf22754c12e28d8251e72842dee77 100644
--- a/webrtc/modules/audio_coding/codecs/audio_encoder.h
+++ b/webrtc/modules/audio_coding/codecs/audio_encoder.h
@@ -25,12 +25,33 @@ namespace webrtc {
 // type must have an implementation of this class.
 class AudioEncoder {
  public:
+  // Used for UMA logging of codec usage. The same codecs in the same
+  // order must be listed in
+  // src/tools/metrics/histograms/histograms.xml in chromium to log
+  // correct values.

[kwiberg-webrtc, 2016/05/12 23:30:21]

Does that file list the actual number for each name, or is the number implicit
from the order there too? If the former is the case (which I would hope, because
it makes mistakes less likely), you should probably use explicit numbers here
too.

[aleloi, 2016/05/13 09:02:27]

Here is what the relevant section of the histogram file looks like:

<enum name="WebRtcAudioCodecs" type="int">
  <int value="0" label="Unknown"/>
  <int value="1" label="Opus"/>
  <int value="2" label="iSAC"/>
  <int value="3" label="pcmA"/>
  <int value="4" label="pcmU"/>
  <int value="5" label="g722"/>
  <int value="6" label="iLBC"/>
</enum>

So yes, it makes sense to use numbers for every codec. I have changed.

+  enum class CodecType {
+    // kOther stands for no name, other or unknown codec
+    kOther = 0,

[kwiberg-webrtc, 2016/05/12 23:30:21]

Maybe

  kOther = 0,  // Codec not specified, and/or not listed in this enum.

?

[aleloi, 2016/05/13 09:02:27]

That's a little more clear, I think. Changed.

+    kOpus,
+    kIsac,
+    kPcmA,
+    kPcmU,
+    kG722,
+    kIlbc,
+
+    // Number of histogram bins in the UMA logging of codec types. The
+    // total number of different codecs that are logged cannot exceed this
+    // number.
+    kMaxLoggedAudioCodecNames = 64

[kwiberg-webrtc, 2016/05/12 23:30:21]

Why is this explicitly 64, rather than just implicitly the next integer? Doesn't
UMA allow us to increase this number later?

[aleloi, 2016/05/13 09:02:27]

I was inspired by the logging of video codecs,
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
I think it's OK to change later.

[Steven Holte, 2016/05/13 21:20:58]

This does seem unnecessary and wastes a tiny bit of memory, since we will
allocate memory to hold 64 counters.  Not horrible, but still odd.  Not sure why
video codecs are logged that way either.

[kwiberg-webrtc, 2016/05/15 02:06:13]

OK. Alex, would you check if we are allowed to change this value later, and drop
the =64 if so?

[aleloi, 2016/05/16 08:02:23]

Done.

+  };
+
   struct EncodedInfoLeaf {
     size_t encoded_bytes = 0;
     uint32_t encoded_timestamp = 0;
     int payload_type = 0;
     bool send_even_if_empty = false;
     bool speech = true;
+    CodecType encoder_type = CodecType::kOther;
   };
 
   // This is the main struct for auxiliary encoding information. Each encoded