Added first layer of the echo canceller 3 functionality

webrtc/modules/audio_processing/aec3/frame_blocker.cc

+/*

[hlundin-webrtc, 2016/12/16 10:04:47]

Order of methods is wrong.

[peah-webrtc, 2016/12/20 10:10:26]

Done.

+ *  Copyright (c) 2016 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.
+ */
+
+#include "webrtc/modules/audio_processing/aec3/frame_blocker.h"
+
+#include <algorithm>
+
+#include "webrtc/base/checks.h"
+#include "webrtc/modules/audio_processing/aec3/aec3_constants.h"
+
+namespace webrtc {
+
+FrameBlocker::FrameBlocker(size_t num_bands, size_t frame_length)
+    : num_bands_(num_bands),
+      frame_length_(frame_length),
+      buffer_(num_bands_ * kBlockSize, 0.f) {}
+
+FrameBlocker::~FrameBlocker() = default;
+
+void FrameBlocker::InsertFrameAndExtractBlock(size_t sub_frame_index,
+                                              rtc::ArrayView<const float> frame,
+                                              rtc::ArrayView<float> block) {
+  RTC_DCHECK_LE(sub_frame_index * kSubFrameLength, frame_length_);

[aleloi, 2016/12/16 15:04:31]

Please DCHECK that the frame is large enough.

[peah-webrtc, 2016/12/20 10:10:25]

I'm not sure what you mean here. The frame is the input, and this method does
not store anything but instead passes the data back on the next
InsertFrameAndExtractBlock method. I'd rather put more DCHECKS there if needed.

WDYT?

+  rtc::ArrayView<const float> frame_view[num_bands_];

[hlundin-webrtc, 2016/12/16 10:04:47]

Please, comment that this is an array of ArrayViews. The sloppy reader might
interpret this as a single array view of size num_bands_.

[peah-webrtc, 2016/12/20 10:10:25]

Done.

+  for (size_t i = 0; i < num_bands_; ++i) {
+    frame_view[i] = rtc::ArrayView<const float>(
+        &frame[i * frame_length_ + kSubFrameLength * sub_frame_index],
+        kSubFrameLength);

[aleloi, 2016/12/16 15:04:31]

I think this does the same thing:
... = frame.subview(i * frame_length_ + kSubFrameLength * sub_frame_index,
kSubFrameLength)

[peah-webrtc, 2016/12/20 10:10:25]

Great! That is definitely better. I changed to that.

Done.

+  }
+  InsertFrameAndExtractBlock(rtc::ArrayView<const rtc::ArrayView<const float>>(

[hlundin-webrtc, 2016/12/16 10:04:47]

An ArrayView of ArrayViews... I think a vector<ArrayView<const float>> would be
better. Pass it as a const ref.

[peah-webrtc, 2016/12/20 10:10:26]

What would be the benefit of that be?  I changed the array size to a compile
time-constant and then that should be faster than a vector allocation.

[hlundin-webrtc, 2016/12/20 15:10:34]

Acknowledged.

[peah-webrtc, 2016/12/21 23:13:49]

Acknowledged.

+                                 &frame_view[0], num_bands_),
+                             block);
+}
+
+void FrameBlocker::InsertFrameAndExtractBlock(size_t sub_frame_index,
+                                              const float* const* frame,
+                                              rtc::ArrayView<float> block) {
+  RTC_DCHECK_LE(sub_frame_index * kSubFrameLength, frame_length_);
+  rtc::ArrayView<const float> frame_view[num_bands_];

[hlundin-webrtc, 2016/12/16 10:04:47]

Again, consider using a vector.

[peah-webrtc, 2016/12/20 10:10:26]

Please see question above.

+  for (size_t i = 0; i < num_bands_; ++i) {
+    frame_view[i] = rtc::ArrayView<const float>(
+        &frame[i][sub_frame_index * kSubFrameLength], kSubFrameLength);
+  }
+  InsertFrameAndExtractBlock(rtc::ArrayView<const rtc::ArrayView<const float>>(
+                                 &frame_view[0], num_bands_),
+                             block);
+}
+
+void FrameBlocker::InsertFrameAndExtractBlock(

[hlundin-webrtc, 2016/12/16 10:04:47]

I need a better explanation of what is supposed to happen here.

[peah-webrtc, 2016/12/20 10:10:26]

True. I added that into the header file.

Done.

+    rtc::ArrayView<const rtc::ArrayView<const float>> frame,

[hlundin-webrtc, 2016/12/16 10:04:47]

Aha! Here is the root of all evil. :)
I would recommend const std::vector<rtc::ArrayView<const float>>& instead.

[peah-webrtc, 2016/12/20 10:10:25]

Please see question above. I don't see why we want to allocate a vector for
that.

+    rtc::ArrayView<float> block) {
+  RTC_DCHECK_LE(buffer_size_, kBlockSize);

[hlundin-webrtc, 2016/12/16 10:04:47]

DCHECK that block is long enough.

[peah-webrtc, 2016/12/20 10:10:25]

Done.

+  RTC_DCHECK_GE(kBlockSize - buffer_size_, kSubFrameLength - kBlockSize);
+

[aleloi, 2016/12/16 15:04:31]

I think this is complex enough to warrant a few unit tests to check that data is
written to the right place of |block|, that there are no reads outside of the
buffer, that band_start_index is updated correctly. If there is one buffer for
each band, it will be a little simpler, though.

[peah-webrtc, 2016/12/20 10:10:26]

I fully agree. I have now changed to one band per buffer, and added unit tests.

PTAL

[aleloi, 2016/12/20 15:55:35]

I like it now!

[peah-webrtc, 2016/12/21 23:13:49]

Great!

+  const size_t samples_to_block = kBlockSize - buffer_size_;
+  size_t band_start_index = 0;
+  for (size_t i = 0; i < num_bands_; ++i) {
+    RTC_DCHECK_EQ(kSubFrameLength, frame[i].size());
+    std::copy(buffer_.begin() + band_start_index,
+              buffer_.begin() + band_start_index + buffer_size_,
+              block.begin() + band_start_index);
+    std::copy(frame[i].begin(), frame[i].begin() + samples_to_block,
+              block.begin() + band_start_index + buffer_size_);
+    std::copy(frame[i].begin() + samples_to_block, frame[i].end(),
+              buffer_.begin() + band_start_index);
+
+    band_start_index += kBlockSize;
+  }
+  buffer_size_ = kSubFrameLength - samples_to_block;
+}
+
+bool FrameBlocker::IsBlockAvailable() {
+  return kBlockSize == buffer_size_;
+}
+
+bool FrameBlocker::ExtractBlockIfAvailable(rtc::ArrayView<float> block) {
+  RTC_DCHECK_GE(kBlockSize, buffer_size_);
+  if (!IsBlockAvailable()) {
+    return false;
+  }
+  size_t band_start_index = 0;
+  for (size_t i = 0; i < num_bands_; ++i) {
+    std::copy(buffer_.begin() + band_start_index,
+              buffer_.begin() + band_start_index + buffer_size_,
+              block.begin() + band_start_index);
+    band_start_index += kBlockSize;
+  }
+  buffer_size_ = 0;
+  return true;
+}
+
+}  // namespace webrtc