Audio codec usage statistics

webrtc/modules/audio_coding/codecs/audio_encoder.cc

Index: webrtc/modules/audio_coding/codecs/audio_encoder.cc
diff --git a/webrtc/modules/audio_coding/codecs/audio_encoder.cc b/webrtc/modules/audio_coding/codecs/audio_encoder.cc
index 2b08dd85940fc2210f3320abe3c9347b54589d53..235612ebe69038924a78c2973c4d4895975cebd7 100644
--- a/webrtc/modules/audio_coding/codecs/audio_encoder.cc
+++ b/webrtc/modules/audio_coding/codecs/audio_encoder.cc
@@ -10,11 +10,57 @@
 
 #include "webrtc/modules/audio_coding/codecs/audio_encoder.h"
 
+#include <cstring>
+#include <cstdlib>
+
 #include "webrtc/base/checks.h"
 #include "webrtc/base/trace_event.h"
+#include "webrtc/system_wrappers/include/metrics.h"
 
 namespace webrtc {
 
+namespace {
+enum HistogramAudioCodecType {

[hlundin-webrtc, 2016/05/11 08:27:26]

Can you use enum class here, or is there something in the UMA mechanics that
prevents it?

+  kUnknown = 0,
+  kOpus = 1,
+  kIsac = 2,
+  kG711 = 3,
+  kG722 = 4,
+  kIlbc = 5,
+  kAudioMax = 64
+};
+
+// On every call to Encode(), the codec used is logged to the
+// histogram with this probability. If Encode() is called every 10ms,
+// the logging will be done approximately once every 5s.
+const float kProbabilityToLogCodecType = 1.0 / 500.0;

[kwiberg-webrtc, 2016/05/11 00:47:00]

constexpr?

[aleloi, 2016/05/11 11:16:13]

Thanks, fixed in next cl

+
+// Translates the name of a codec returned by GetCodecName() into
+// one of the codec ID:s in CodecID

[kwiberg-webrtc, 2016/05/11 00:47:00]

"ID:s" -> "IDs".

What is CodecID?

[aleloi, 2016/05/11 11:16:13]

CodecID was HistogramAudioCodecType earlier. I changed to be consistent with
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

Since we are considering to remove the randomness, this may not be relevant any
more.

+HistogramAudioCodecType CodecNameToHistogramCodecType(const char* codec_name) {
+  if (strcmp("Opus", codec_name) == 0)

[kwiberg-webrtc, 2016/05/11 00:47:00]

codec_name can be null, but the behavior of strcmp is undefined if you pass it
anything but pointers to null-terminated strings.

[hlundin-webrtc, 2016/05/11 08:27:26]

We should make sure that the naming is consistent with _some_ standard. I think
looking at the SDP "encoding name" value for different codecs is good. That
would give us:
- "opus" (https://tools.ietf.org/html/draft-ietf-payload-rtp-opus-11#section-7)
- "isac-16000" and "isac-32000"
(https://tools.ietf.org/html/draft-ietf-payload-rtp-opus-11#section-7),
- "iLBC" (https://tools.ietf.org/html/rfc3952#section-5),
- Bunch of "fixed payload types defined in
https://tools.ietf.org/html/rfc3551#section-4.5
-- G722,
-- PCMA (for g711A),
-- PCMU (for g711u),
-- L16-8000 for PCM16@8kHz, etc. 

The formatting is a bit mixed, I agree, but I think it is nice to stick to some
standard. WDYT?

[aleloi, 2016/05/11 11:16:13]

Thanks, fixed in next cl. I took the names from the design document. I will
rename when you have agreed.

[hlundin-webrtc, 2016/05/11 13:03:55]

Acknowledged.

+    return kOpus;
+  else if (strcmp("iSac", codec_name) == 0)

[kwiberg-webrtc, 2016/05/11 00:47:00]

iSAC

[aleloi, 2016/05/11 11:16:13]

Thanks, fixed in next CL

+    return kIsac;
+  else if (strcmp("g711", codec_name) == 0)
+    return kG711;
+  else if (strcmp("g722", codec_name) == 0)
+    return kG722;
+  else if (strcmp("iLBC", codec_name) == 0)
+    return kIlbc;
+  else
+    return kUnknown;
+}

[kwiberg-webrtc, 2016/05/11 00:47:00]

Hmm. You end up having to basically maintain two copies of the codec list, one
enum and one in the code. Do you think a table-driven approach like this might
work well?:

static const struct {
  const char* name;
  int id;
} histogram_id = {
  {"Opus", 1},
  ...
};
for (const auto& codec : histogram_id) {
  if (std::strcmp(codec.name, codec_name) == 0)
    return codec.id;
}

[aleloi, 2016/05/11 11:16:13]

Is there a reason for declaring the 'histogram_id' array 'static const' rather
that 'const' or 'constexpr'?


The array declaration was a little hard for me to read. Would it be better to
move the type definition out like this:

  struct CodecNameIDPair {
    const char* name;
    int id;
  };

  static const const CodecNameIDPair histogram_id[] = {
    {"Opus", 1},
    ...
  };

What do you think about replacing the struct type with std::pair<const char*,
int>? I think it may be shorter and more readable. Since initializer lists can
construct pairs, it will not add much syntactical overhead. But it seems that
the array 'histogram_id'  cannot be made constexpr with the current compiler
version and flags. (It works in g++ 4.8.4 -std=c++11)

[kwiberg-webrtc, 2016/05/11 12:00:41]

static const means it's a global constant that doesn't get reinitialized every
time you enter its scope.

I haven't tried it, but I suspect the char pointers can't be constexpr since the
value of the pointers may not be known at compile time. But I may be wrong.
Well, then you have to name the struct, but sure, you can do that.
The good thing about using a struct is that the members have meaningful names.
So I like this variant less than the other two.

+
+// Adds a codec usage sample to the histogram.
+void UpdateCodecTypeHistogram(const char* codec_name) {
+  RTC_HISTOGRAM_ENUMERATION("WebRTC.Audio.Encoder.CodecType",
+                            CodecNameToHistogramCodecType(codec_name),
+                            kAudioMax);
+}
+
+}  // namespace
+
 AudioEncoder::EncodedInfo::EncodedInfo() = default;
 AudioEncoder::EncodedInfo::EncodedInfo(const EncodedInfo&) = default;
 AudioEncoder::EncodedInfo::EncodedInfo(EncodedInfo&&) = default;
@@ -36,6 +82,9 @@ AudioEncoder::EncodedInfo AudioEncoder::Encode(
   RTC_CHECK_EQ(audio.size(),
                static_cast<size_t>(NumChannels() * SampleRateHz() / 100));
 
+  if (rand() / RAND_MAX < kProbabilityToLogCodecType)

[kwiberg-webrtc, 2016/05/11 00:47:00]

std::rand

Also, since std::rand() returns an int, and RAND_MAX is an int, the quotient
will always be 0, so the condition will always be true. Maybe try something like

  constexpr int threshold = RAND_MAX * kProbabilityToLogCodecType;
  if (rand() < threshold)

[the sun, 2016/05/11 08:18:43]

We shouldn't be using std::rand() since:
a) it relies on shared global state
b) the PRNG used is implementation dependent so we have no control over how fair
the generator is (it may even skew results depending on platform).
c) the implementations I've seen use a simple mult+add which is known to not
generate very long cycles.

The alternative is to add the PRNG in the class, but then we have state again so
might as well resort to using an int counter like the design doc proposes.

Once we have injectable encoders, we'll nevertheless be able to pull this
bookkeeping from the public interface and keep it in an internal decorator
object, so I think we can live with adding state to the class for now.

[kwiberg-webrtc, 2016/05/11 08:52:24]

Yeah, that's suboptimal because locking.
I haven't done the math, but it seems like you'd need a very poor PRNG indeed to
botch the job here. After all, we're almost just as happy with a "PRNG" that
simply says no 499 times, then yes once, then repeats.
Or to have a thread-local PRNG.
There's nothing stopping us from doing that right away, is there? We can just
put the state (and the logging) at the call site (IIRC from our discussions,
there is only one call site that matters). That's probably a pragmatic approach
to getting this CL landed in a timely manner...

[the sun, 2016/05/11 10:35:01]

Two sites: Encode() and dtor.

[hlundin-webrtc, 2016/05/11 10:37:20]

dtor only if we care about the "remainder" (fraction of 5 seconds) that is
unlogged on destruction. All codecs will suffer the same bias if we omit the
dtor logging. Just saying, if the dtor logging makes the solution much more
complicated.

[kwiberg-webrtc, 2016/05/11 10:46:21]

Short-lived codecs will have a different bias than long-lived codecs if you use
a counter and don't update it in the destructor. But unless my math intuition
fails me, this will not be the case if we log randomly.

[hlundin-webrtc, 2016/05/11 11:01:37]

Are some codecs expected to live shorter than others?

[aleloi, 2016/05/11 11:16:13]

I talked with solenberg@ about using randomness. If I understood correctly,
there were several drawbacks:

1. rand() has state, and we would like to keep the encoder state-less
2. rand() shares its state across all threads
3. it is currently not given a seed
4. it is a relatively bad random number generator

The conclusion was that since we need state anyway, we can as well put a counter
in a private member variable.

Another matter is that (contrary to what we believed and said earlier) the
random trial has quite bad statistic properties. E.g. if you want to be 90% sure
that the counter will be more than 10% off, you have to run the encoder for at
least 8 days. (Here is the math:
https://drive.google.com/a/google.com/file/d/0B0ohpnzd-DTsTkJyZ29OdmpEdVk/vie...
)

[kwiberg-webrtc, 2016/05/11 12:00:41]

Not an issue. rand's state is global.
Yes, bad for performance reasons.
Possibly relevant, but easily solvable by using a better PRNG.
I really don't like to put state in interfaces. Much preferable to put the state
outside the class in that case.
Well mathed! So tl;dr is that the expected value has all the properties we want,
but the variance is high?

However, this is the total time for all users, right? Not per user? Then 8 days
isn't necessarily all that much. But what's the story if we want e.g. 99%
confidence that we got within 1%? Am I right that no reasonable number of users
will save us then?

+    UpdateCodecTypeHistogram(GetCodecName());
+
   const size_t old_size = encoded->size();
   EncodedInfo info = EncodeImpl(rtp_timestamp, audio, encoded);
   RTC_CHECK_EQ(encoded->size() - old_size, info.encoded_bytes);
@@ -60,9 +109,12 @@ void AudioEncoder::SetProjectedPacketLossRate(double fraction) {}
 
 void AudioEncoder::SetTargetBitrate(int target_bps) {}
 
+const char* AudioEncoder::GetCodecName() const {
+  return nullptr;
+}
+
 size_t AudioEncoder::MaxEncodedBytes() const {
   RTC_CHECK(false);
   return 0;
 }
-
 }  // namespace webrtc