Audio codec usage statistics

webrtc/modules/audio_coding/codecs/audio_encoder.h

 /*
  *  Copyright (c) 2014 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_CODING_CODECS_AUDIO_ENCODER_H_
 #define WEBRTC_MODULES_AUDIO_CODING_CODECS_AUDIO_ENCODER_H_
 
 #include <algorithm>
 #include <vector>
 
 #include "webrtc/base/array_view.h"
 #include "webrtc/base/buffer.h"
 #include "webrtc/base/deprecation.h"
 #include "webrtc/typedefs.h"
 
 namespace webrtc {
 
 // This is the interface class for encoders in AudioCoding module. Each codec
 // 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
   // packet should be accompanied by one EncodedInfo struct, containing the
   // total number of |encoded_bytes|, the |encoded_timestamp| and the
   // |payload_type|. If the packet contains redundant encodings, the |redundant|
   // vector will be populated with EncodedInfoLeaf structs. Each struct in the
   // vector represents one encoding; the order of structs in the vector is the
   // same as the order in which the actual payloads are written to the byte
   // stream. When EncoderInfoLeaf structs are present in the vector, the main
@@ skipping 96 matching lines @@
   // This function is deprecated. It was used to return the maximum number of
   // bytes that can be produced by the encoder at each Encode() call.  Since the
   // Encode interface was changed to use rtc::Buffer, this is no longer
   // applicable. It is only kept in to avoid breaking subclasses that still have
   // it implemented (with the override attribute). It will be removed as soon
   // as these subclasses have been given a chance to change.
   virtual size_t MaxEncodedBytes() const;
 };
 }  // namespace webrtc
 #endif  // WEBRTC_MODULES_AUDIO_CODING_CODECS_AUDIO_ENCODER_H_