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..24006fdf674b332daf8c07c8037a44285460149f 100644
--- a/webrtc/modules/audio_coding/codecs/audio_encoder.h
+++ b/webrtc/modules/audio_coding/codecs/audio_encoder.h
@@ -136,6 +136,9 @@ class AudioEncoder {
                                  rtc::ArrayView<const int16_t> audio,
                                  rtc::Buffer* encoded) = 0;
 
+  // Subclasses implements this and return a name, e.g. "Opus" or "iSAC".

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

Despite the example set by EncodeImpl, you probably don't need to say that
subclasses can implement the method---the "virtual" already says that. But it
would be useful to document the lifetime of the returned string, and what the
default implementation does. Maybe

  // Returns a pointer to a statically allocated string with the codec name,
  // e.g. "Opus" or "iSAC". The default implementation returns null,
  // which means no name.

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

I'm not sure we should promise statically allocated strings. We talked about
wanting to distinguish between different codec settings, e.g., Opus with and
without DTX enabled. Also, for both iSAC and the PCM16 "codec", the sample rate
may vary.

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

OK. What lifetime should we promise, then? That of the AudioEncoder instance?

[hlundin-webrtc, 2016/05/11 08:59:18]

That seems reasonable, but promising the correct value is another issue. I mean,
_if_ we are going to distinguish between Opus with and without DTX, that is
going to be a runtime parameter. So, correct string value can only be guaranteed
until the settings change. Can we be conservative and just promise lifetime
until the next call to any other method (or lifetime, whichever is shorter)?

[kwiberg-webrtc, 2016/05/11 09:49:43]

I think it's problematic if we have an interface that requires the encoder to
generate these strings on the fly. That's costly, and besides means there are a
lot of them, which will cause problems for the consumer (remember the limit of
64 codec names from elsewhere in this CL?).

If the number of possible names an encoder can return is small, they can all be
kept in a table with encoder instance or even static life time.

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

I too was thinking of a "small set" of statically defined strings. But still,
the output from GetCodecName will not be statically known, and the
correctness-time of the string may be shorter than the lifetime of the object.

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

But the complete set of strings possibly returned by the encoder needs to be
known by the caller, so it can't be so large that it's infeasible for the
encoder to just store them all in a small table.

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

Acknowledged.

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

Can a C++ string be returned dynamically instead? Then one does not have to
worry about lifetime and deallocation.

[The Sun (google.com), 2016/05/11 11:36:44]

A string returned by value will likely require heap allocations, which is what
we want to avoid here (some string implementations won't use heap allocs for
short strings though, which we certainly have here, but it is fragile to depend
on std lib implementations).

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

I agree. We definitely want to return a pointer to a string that's no on the
heap. And my point is that there should be few enough of them that they can be
stored in a small static table, which implies we gain nothing by allowing the
lifetime of the strings to be limited.

+  virtual const char* GetCodecName() const;
+
  private:
   // 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