Audio codec usage statistics

webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc

Index: webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc
diff --git a/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc b/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc
index 1a1ae37a8a38da869caa30fc78539ccb49c11028..999558298e603f1b35ae37f4597daf61d7429e1c 100644
--- a/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc
+++ b/webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc
@@ -29,6 +29,57 @@
 
 namespace webrtc {
 
+namespace {
+
+struct CodecNameIDPair {

[hlundin-webrtc, 2016/05/12 12:23:39]

The style guide says you should write CodecNameIdPair (with small 'd').
https://google.github.io/styleguide/cppguide.html#Function_Names
" Prefer to capitalize acronyms as single words (i.e. StartRpc(), not
StartRPC())."

+  const char* name;
+  size_t id;
+};

[kwiberg-webrtc, 2016/05/12 13:01:51]

Please document. Alternatively, move this definition to just before the one
place where it's used (inside CodecNameToHistogramCodecType).

+
+
+// Ensures that every element of passed array har .id field less than

[hlundin-webrtc, 2016/05/12 12:23:39]

har?

+// kMaxCodecNames and that count starts from '1' with 1-step
+// increments
+template<int N>
+constexpr bool idSLessThanMaxCodecNames(const CodecNameIDPair (&array) [N],

[hlundin-webrtc, 2016/05/12 12:23:39]

Name formatting...

+                                        size_t index) {
+  return (index >= N) ||
+      (array[index].id < acm2::AudioCodingModuleImpl::kMaxAudioCodecNames &&
+       array[index].id == index + 1 &&
+       idSLessThanMaxCodecNames(array, index + 1));
+}

[kwiberg-webrtc, 2016/05/12 13:01:51]

This is a lot of hard-to-read code, and it doesn't even check that ID's are >=1
or that you don't use the same ID twice. I'd argue that it isn't worth it.

[aleloi, 2016/05/12 13:25:46]

I claim it does check that ID:s are different and >= 1. If it returns 'true' on
index 'i', it means that every pair at indices [i, i+1, ... N-1] has id exactly
'index+1' (thus no repetitions) and is less than the number of bins. Since
'index' is at least 0, they must be all at least 1.

But I agree that it's probably not worth it.

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

Oh, I see. I didn't read it carefully, just enough to see that you only checked
each entry in isolation, which wouldn't be enough to verify what I had in mind
(that the first element was >0, that each element was greater than the one
before it, and that the last element was <max). But it is enough to check this
stricter property.

+
+// Translates the name of a codec returned by GetCodecName() into

[kwiberg-webrtc, 2016/05/12 13:01:51]

That's no longer where the names come from.

[aleloi, 2016/05/12 13:25:47]

Acknowledged.

+// one of the codec IDs in the table 'histogram_id' below.
+size_t CodecNameToHistogramCodecType(const char* codec_name) {
+  constexpr CodecNameIDPair histogram_id[] = {

[kwiberg-webrtc, 2016/05/12 13:01:51]

static constexpr. Otherwise the compiler might put the table on the stack each
time you call this function, I believe.

[aleloi, 2016/05/12 13:25:46]

Done.

+    {"Opus", 1},

[kwiberg-webrtc, 2016/05/12 13:01:51]

Add a comment before the first entry explaining what 0 means.

[aleloi, 2016/05/12 13:25:47]

Done.

+    {"iSAC", 2},
+    {"g711A", 3},
+    {"g711U", 4},
+    {"g722", 5},
+    {"iLBC", 6}

[kwiberg-webrtc, 2016/05/12 13:01:51]

Use a trailing comma here too. Otherwise, the diff will look ugly when you add
new stuff to the end of the table.

[aleloi, 2016/05/12 13:25:47]

I didn't know that was allowed in C++! Always learning new stuff...

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

I've never actually checked what the standard says, but if all our trybots
accept it (which I think they will) you should go ahead and use it IMO, since
it's rather useful.

+  };
+
+  static_assert(idSLessThanMaxCodecNames(histogram_id, 0),
+                  "Cannot guarantee that codec ID does not exceed boundary.");
+
+  for (const auto & codec : histogram_id) {

[kwiberg-webrtc, 2016/05/12 13:01:51]

No space between auto and &. (Did you run git cl format?)

[aleloi, 2016/05/12 13:25:47]

Nope, forgot!

+    if (codec_name != nullptr && strcmp(codec.name, codec_name) == 0)

[ossu, 2016/05/12 12:50:14]

You should check codec_name before entering the loop. No reason to go through
that every pass.

[kwiberg-webrtc, 2016/05/12 13:01:51]

Pointers are true iff they're not null, so just

  if (codec_name && ...

works just as well.

[aleloi, 2016/05/12 13:25:46]

Done.

[aleloi, 2016/05/12 13:25:47]

I followed ossu's recommendation and moved the check outside. It is now 

if (codec_name == nullptr) ...

which I think is more readable than "if (!codec_name)"

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

OK. You're not alone in inexplicably preferring the longer form, so I'll just
note that my recommendation still stands and try to not make a scene. :-)

+      return codec.id;
+  }
+  return 0;
+}
+
+// Adds a codec usage sample to the histogram.
+void UpdateCodecTypeHistogram(size_t codec_type) {
+  RTC_HISTOGRAM_ENUMERATION("WebRTC.Audio.Encoder.CodecType",
+                            codec_type,
+                            acm2::AudioCodingModuleImpl::kMaxAudioCodecNames);
+}
+}  // namespace
+
+
 namespace acm2 {
 
 struct EncoderFactory {
@@ -185,7 +236,9 @@ AudioCodingModuleImpl::AudioCodingModuleImpl(
       first_10ms_data_(false),
       first_frame_(true),
       packetization_callback_(NULL),
-      vad_callback_(NULL) {
+      vad_callback_(NULL),
+      codec_histogram_bins_log(),

[kwiberg-webrtc, 2016/05/12 13:01:51]

Don't you need to fill this with zeros?

[aleloi, 2016/05/12 13:25:47]

It gets filled with zeroes with codec_histogram_bins_log():

http://stackoverflow.com/a/6499281

citation: 
  if T is an array type, then each element is value-initialized;
  An object whose initializer is an empty set of parentheses, i.e., (), shall be
value-initialized.

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

Right. Thanks!

+      number_of_consecutive_empty_packets(0) {
   if (InitializeReceiverSafe() < 0) {
     WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceAudioCoding, id_,
                  "Cannot initialize receiver");
@@ -231,6 +284,21 @@ int32_t AudioCodingModuleImpl::Encode(const InputData& input_data) {
   }
   previous_pltype = previous_pltype_;  // Read it while we have the critsect.
 
+  // Log codec type to histogram once every 500 packets.
+  if (encoded_info.encoded_bytes == 0) {
+    number_of_consecutive_empty_packets += 1;

[ossu, 2016/05/12 12:50:14]

I'd prefer ++number_of_consecutive_empty_packets. ++ incrementation seems to be
the most widely used form in the codebase.

[aleloi, 2016/05/12 13:25:47]

Done.

+  }
+  else {

[ossu, 2016/05/12 12:50:14]

else on the same line as the closing curly seems to be preferred by the style
guide. Also by me. :)

[aleloi, 2016/05/12 13:25:47]

Done.

+    size_t codec_type =

[ossu, 2016/05/12 12:50:14]

So the codec id is looked up based on its name once for each packet? Seems a bit
expensive. With just six formats its probably fine but if the list were
expanded, maybe a faster lookup would be preferred (like a std::map<string,
size_t>, for example).

[kwiberg-webrtc, 2016/05/12 13:01:51]

I'm guessing we'd need quite a lot of entries before std::map outperformed an
array... More than we'll probably ever end up using. Keeping the array until
such time as we unexpectedly find ourselves wanting to keep statistics for a
hundred codecs seems like a wise choice.

[aleloi, 2016/05/12 13:25:47]

We only do trivial operations in the lookup. A map lookup also does lots of
comparisons (which have linear cost in the length of the strings). My gut
feeling says that may<string> lookup (which is implemented as a red-black tree,
I think) is only faster for double-digit amounts of formats.

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

Yes. std::map will also do a lot of cache-unfriendly pointer chasing. If we do
end up with a table with 100+ rather than ~10 entries, std::unordered_map would
probably be the data structure of choice.

+        CodecNameToHistogramCodecType(encoded_info.encoder_name);
+    codec_histogram_bins_log[codec_type] += 1;
+    number_of_consecutive_empty_packets = 0;

[kwiberg-webrtc, 2016/05/12 13:01:51]

Here you zero number_of_consecutive_empty_packets without having used the value.
:-)

[aleloi, 2016/05/12 13:25:47]

Acknowledged.

+    if (codec_histogram_bins_log[codec_type] >= 500) {
+      codec_histogram_bins_log[codec_type] -= 500;
+      UpdateCodecTypeHistogram(codec_type);
+    }
+  }
+
   RTPFragmentationHeader my_fragmentation;
   ConvertEncodedInfoToFragmentationHeader(encoded_info, &my_fragmentation);
   FrameType frame_type;