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.
  */
 
@@ skipping 118 matching lines @@
   // implementation does the latter).
   virtual void SetTargetBitrate(int target_bps);
 
  protected:
   // Subclasses implement this to perform the actual encoding. Called by
   // Encode().
   virtual EncodedInfo EncodeImpl(uint32_t rtp_timestamp,
                                  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
   // 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_