Add mute state field to AudioFrame.

webrtc/modules/include/module_common_types.h

Index: webrtc/modules/include/module_common_types.h
diff --git a/webrtc/modules/include/module_common_types.h b/webrtc/modules/include/module_common_types.h
index 98f7a38af204fe32a17e7c5f84b9d205c203dc2a..62874a03aee85b235a4c02ce0ab090d094d33bab 100644
--- a/webrtc/modules/include/module_common_types.h
+++ b/webrtc/modules/include/module_common_types.h
@@ -271,11 +271,8 @@ class CallStatsObserver {
  * states.
  *
  * Notes
- * - The total number of samples in |data_| is
- *   samples_per_channel_ * num_channels_
- *
+ * - The total number of samples is samples_per_channel_ * num_channels_
  * - Stereo data is interleaved starting with the left channel.
- *
  */
 class AudioFrame {
  public:
@@ -299,8 +296,7 @@ class AudioFrame {
 
   AudioFrame();
 
-  // Resets all members to their default state (except does not modify the
-  // contents of |data_|).
+  // Resets all members to their default state.
   void Reset();
 
   void UpdateFrame(int id, uint32_t timestamp, const int16_t* data,
@@ -310,11 +306,21 @@ class AudioFrame {
 
   void CopyFrom(const AudioFrame& src);
 
+  // data() returns a zeroed static buffer if the frame is muted.
+  // mutable_frame() always returns a non-static buffer; the first call to
+  // mutable_frame() zeros the non-static buffer and marks the frame unmuted.
+  const int16_t* data() const;

[hlundin-webrtc, 2017/03/16 14:47:48]

This is probably going to break all the work you've done so far, but I wonder if
we actually would like to make this return an rtc::ArrayView<const int16_t> (and
maybe an rtc::ArrayView<int16_t> below, but that poses even more problems).
Pros:
+ The actual length of the data can be included in the type. No need to look at
samples per channel.
+ Modern C++ things like range-based for loops work out of the box.
Cons:
- Breaks even more code.
- Might result in asymmetric APIs if both of these cannot be made to return
ArrayViews.

The problem with mutable_data is that the caller might want to change the number
of samples (like when upsampling). This does not work with the ArrayView design.


Another alternative is to change the member data_ to be an rtc::Buffer<int16_t>.
Then data() can return a const ref/ptr (or just as easily an rtc::ArrayView),
and mutable_data() can return a pointer to the Buffer object. The caller can
then change the size when needed.

I realize that this really increases the scope of this change, and I'd rather
not force this on you.

WDYT?

[hlundin-webrtc, 2017/03/16 14:58:23]

Actually, after discussions with Solenberg, I changed my mind. Let's not bother
with this now. Keep the changes pragmatic for now, and we will revisit the
design questions at a later stage. Feel free to disregard from my comment above.

[yujo, 2017/03/16 23:37:21]

I considered the ArrayView approach, but ran into the same issue with changing
the length. And using Buffer means allocations on resizes, which perhaps is best
avoided.

Maybe introduce a version of mutable_frame() that takes num_channels,
samples_per_channel, and sample_rate_hz? If we add the corresponding version of
Mute(), then we can hide those fields entirely. That would be manageable to do
in this change, if you think it's an acceptable solution.

[hlundin-webrtc, 2017/03/17 14:29:38]

Let's take this one step at a time, and skip the ArrayView for now. The
AudioFrame is slated for more refactoring in the future, so we'll have plenty of
opportunities to figure out great designs then.

[kwiberg-webrtc, 2017/05/10 08:38:28]

(No need to change anything in this CL as a result of my comments, since I'm
very late to the party.)

I think data() should definitely return an ArrayView<const int16_t>. It's easier
to not make mistakes about the length of arrays if the pointer and the length
are always bundled in a standardized way. Callers that really want just the
pointer can say ".data().data()"...

For mutable data, using an rtc::Buffer wouldn't have to cause extra
allocations---just make sure to initialize it with a capacity of
kMaxDataSizeSamples and you're golden. So returning a Buffer<int16_t>* (or
possibly Buffer<int16_t>&) seems fine.

+  int16_t* mutable_data();
+
+  // Prefer to mute frames using AudioFrameOperations::Mute.
+  void Mute();
+  // Frame is muted by default.
+  bool muted() const;
+
   // These methods are deprecated. Use the functions in
   // webrtc/audio/utility instead. These methods will exists for a
   // short period of time until webrtc clients have updated. See
   // webrtc:6548 for details.
-  RTC_DEPRECATED void Mute();
   RTC_DEPRECATED AudioFrame& operator>>=(const int rhs);
   RTC_DEPRECATED AudioFrame& operator+=(const AudioFrame& rhs);
 
@@ -327,7 +333,6 @@ class AudioFrame {
   // NTP time of the estimated capture time in local timebase in milliseconds.
   // -1 represents an uninitialized value.
   int64_t ntp_time_ms_ = -1;
-  int16_t data_[kMaxDataSizeSamples];
   size_t samples_per_channel_ = 0;
   int sample_rate_hz_ = 0;
   size_t num_channels_ = 0;
@@ -335,14 +340,27 @@ class AudioFrame {
   VADActivity vad_activity_ = kVadUnknown;
 
  private:

[yujo, 2017/03/16 23:37:21]

There's a presubmit warning about this being a public header file. Is there any
clean way to do the deprecation here?

[hlundin-webrtc, 2017/03/17 14:29:38]

Yeah, this is going to be a bit of a pain. Let's finalize the CL first, and then
we can discuss offline how to go about landing this.

+  // A permamently zeroed out buffer to represent muted frames. This is a
+  // header-only class, so the only way to avoid creating a separate empty
+  // buffer per translation unit is to wrap a static in an inline function.
+  static const int16_t* empty_data() {
+    static const int16_t kEmptyData[kMaxDataSizeSamples] = {};

[hlundin-webrtc, 2017/03/16 14:47:48]

Nit: I think an explicit {0} would be more obvious to the casual reader (unless
there is a performance penalty to it).

[yujo, 2017/03/16 23:37:21]

Done.

+    return kEmptyData;
+  }
+
+  int16_t data_[kMaxDataSizeSamples];
+  bool muted_ = true;
+
+ public:

[hlundin-webrtc, 2017/03/16 14:47:48]

Hmmm. Two public sections...
I think you can safely declare this along with kMaxDataSizeSamples, and let it
be kMaxDataSizeSamples * 2.

Also, this thing with an enum for constants. We tend not to do this anymore. Use
a constexpr instead.

[yujo, 2017/03/16 23:37:21]

Yeah, made it a static_assert instead and moved the constant up top.
I did try, but for some reason the compiler treats it as a static variable
rather than a compile-time constant, which means linking fails because the
symbol is missing. It works with an out-of-class definition, but because this is
a header-only class and we don't get inline variables until C++17, that would
waste a few KB of binary size and runtime memory.

[hlundin-webrtc, 2017/03/17 14:29:39]

Excellent!

+  enum : size_t {
+    kMaxDataSizeBytes = sizeof(data_)
+  };
+
+ private:
   RTC_DISALLOW_COPY_AND_ASSIGN(AudioFrame);
 };
 
-// TODO(henrik.lundin) Can we remove the call to data_()?
-// See https://bugs.chromium.org/p/webrtc/issues/detail?id=5647.
-inline AudioFrame::AudioFrame()
-    : data_() {
-}
+inline AudioFrame::AudioFrame() {}
 
 inline void AudioFrame::Reset() {
   id_ = -1;
@@ -351,6 +369,7 @@ inline void AudioFrame::Reset() {
   timestamp_ = 0;
   elapsed_time_ms_ = -1;
   ntp_time_ms_ = -1;
+  muted_ = true;
   samples_per_channel_ = 0;
   sample_rate_hz_ = 0;
   num_channels_ = 0;
@@ -376,10 +395,10 @@ inline void AudioFrame::UpdateFrame(int id,
 
   const size_t length = samples_per_channel * num_channels;
   assert(length <= kMaxDataSizeSamples);
-  if (data != NULL) {
+  if (data != nullptr) {
     memcpy(data_, data, sizeof(int16_t) * length);
   } else {
-    memset(data_, 0, sizeof(int16_t) * length);
+    muted_ = true;
   }
 }
 
@@ -390,6 +409,7 @@ inline void AudioFrame::CopyFrom(const AudioFrame& src) {
   timestamp_ = src.timestamp_;
   elapsed_time_ms_ = src.elapsed_time_ms_;
   ntp_time_ms_ = src.ntp_time_ms_;
+  muted_ = src.muted();
   samples_per_channel_ = src.samples_per_channel_;
   sample_rate_hz_ = src.sample_rate_hz_;
   speech_type_ = src.speech_type_;
@@ -398,16 +418,35 @@ inline void AudioFrame::CopyFrom(const AudioFrame& src) {
 
   const size_t length = samples_per_channel_ * num_channels_;
   assert(length <= kMaxDataSizeSamples);
-  memcpy(data_, src.data_, sizeof(int16_t) * length);
+  if (!src.muted()) {
+    memcpy(data_, src.data(), sizeof(int16_t) * length);
+  }
+}
+
+inline const int16_t* AudioFrame::data() const {
+  return muted_ ? empty_data() : data_;
+}
+
+// TODO(henrik.lundin) Can we skip zeroing the buffer?
+// See https://bugs.chromium.org/p/webrtc/issues/detail?id=5647.
+inline int16_t* AudioFrame::mutable_data() {
+  if (muted_) {
+    memset(data_, 0, kMaxDataSizeBytes);
+    muted_ = false;
+  }
+  return data_;
 }
 
 inline void AudioFrame::Mute() {
-  memset(data_, 0, samples_per_channel_ * num_channels_ * sizeof(int16_t));
+  muted_ = true;
 }
 
+inline bool AudioFrame::muted() const { return muted_; }
+
 inline AudioFrame& AudioFrame::operator>>=(const int rhs) {
   assert((num_channels_ > 0) && (num_channels_ < 3));
   if ((num_channels_ > 2) || (num_channels_ < 1)) return *this;
+  if (muted_) return *this;
 
   for (size_t i = 0; i < samples_per_channel_ * num_channels_; i++) {
     data_[i] = static_cast<int16_t>(data_[i] >> rhs);
@@ -420,8 +459,9 @@ inline AudioFrame& AudioFrame::operator+=(const AudioFrame& rhs) {
   assert((num_channels_ > 0) && (num_channels_ < 3));
   if ((num_channels_ > 2) || (num_channels_ < 1)) return *this;
   if (num_channels_ != rhs.num_channels_) return *this;
+  if (rhs.muted()) return *this;
 
-  bool noPrevData = false;
+  bool noPrevData = muted_;
   if (samples_per_channel_ != rhs.samples_per_channel_) {
     if (samples_per_channel_ == 0) {
       // special case we have no data to start with
@@ -440,8 +480,9 @@ inline AudioFrame& AudioFrame::operator+=(const AudioFrame& rhs) {
 
   if (speech_type_ != rhs.speech_type_) speech_type_ = kUndefined;
 
+  muted_ = false;
   if (noPrevData) {
-    memcpy(data_, rhs.data_,
+    memcpy(data_, rhs.data(),
            sizeof(int16_t) * rhs.samples_per_channel_ * num_channels_);
   } else {
     // IMPROVEMENT this can be done very fast in assembly