Added first layer of the echo canceller 3 functionality

webrtc/modules/audio_processing/aec3/block_framer.cc

+/*
+ *  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/block_framer.h"
+
+#include <algorithm>
+
+#include "webrtc/base/checks.h"
+
+namespace webrtc {
+
+BlockFramer::BlockFramer(size_t num_bands, size_t frame_length)

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

I'm having some issues with the implementation of this class.

It seems to me that you are storing all bands in one and the same vector, but
never utilize the fact that they reside in some kind of contiguous memory. So
the single vector approach just makes the indexing harder. I would suggest that
you have num_bands vectors instead. Then you could also consider using circular
arrays/buffers instead of plain vectors, and avoid the shifting.

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

+1 Does anybody know if we have a stock circular buffer somewhere? I found
base/bufferqueue.h, but it seems to have some draw-backs. Only bytes can be
read/written and it's thread-safe.

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

We have a stock circular buffer available, but that is C-based and does not have
a nice implementation.  I'll change the code to not have the extra shifting of
the data. I'm not sure whether it would be faster to have a circular buffer
though. 

PTAL

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

Good point! I altered to vectors of vectors.

Regarding the circular buffers I don't think that anything is gained by that.
Basically by shifting you refer to the  copy on line 53, right? 

My concern with circular buffers is that the wraparound behavior and length of
the input and outputs create a fairly complicated wraparound behavior.
That means that you either get really complicated code with a number of
for-loops or need to use a circularly updated buffer index variable (of the form
k = (k+1)%size).
My feeling is that loops with circularly updated buffer indices are quite hard
for the compiler to do efficiently as the memory that is copied is not
continuous.
Therefore I think that it is better to do the copying as several memcopies
instead, which should be very efficient.

WDYT?




I don't think we gain anyth

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

Acknowledged. The new version looks very good!

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

Great!

+    : num_bands_(num_bands),
+      frame_length_(frame_length),
+      buffer_(num_bands_ * kMaxSize, 0.f),
+      buffer_size_(kBlockSize) {}
+
+BlockFramer::~BlockFramer() = default;
+
+void BlockFramer::InsertBlock(rtc::ArrayView<const float> block) {
+  RTC_DCHECK_LE(buffer_size_ + kBlockSize, kMaxSize);

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

There is an assumption here that the block is (at least) num_bands_*kBlockSize
large, right? I think we could add a check for the size of |block|, since
ArrayView::subview doesn't have any bound checking.

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

Done.

+  for (size_t i = 0; i < num_bands_; ++i) {
+    rtc::ArrayView<const float> band =
+        block.subview(i * kBlockSize, kBlockSize);
+    std::copy(band.begin(), band.end(),
+              buffer_.begin() + i * kMaxSize + buffer_size_);
+  }
+  buffer_size_ += kBlockSize;
+}
+
+void BlockFramer::ExtractFrame(size_t sub_frame_index, float* const* frame) {

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

Is there some way to avoid the naked pointers in the output argument? Is it
possible to make |frame| be a vector of ArrayViews instead that is filled by
this method? 

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

That is a great point! I did that instead by wrapping the frames into arrayviews
before the call to BlockFramer and FrameBlocker.

PTAL

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

Acknowledged.

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

Acknowledged.

+  RTC_DCHECK_LE(kSubFrameLength, buffer_size_);
+  RTC_DCHECK_LE(sub_frame_index * kSubFrameLength, frame_length_);
+  for (size_t i = 0; i < num_bands_; ++i) {
+    rtc::ArrayView<float> frame_view(
+        &frame[i][kSubFrameLength * sub_frame_index], kSubFrameLength);
+    std::copy(buffer_.begin() + i * kMaxSize,
+              buffer_.begin() + i * kMaxSize + kSubFrameLength,
+              frame_view.begin());
+  }
+  buffer_size_ -= kSubFrameLength;
+
+  if (buffer_size_ > 0) {

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

Add a comment that the code below shifts the remaining part of the buffer to the
start.

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

Done.

+    RTC_DCHECK_LE(buffer_size_, kMaxSize);

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

This should always be true; move up to beginning of method.

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

Done.

+    for (size_t i = 0; i < num_bands_; ++i) {
+      std::copy(buffer_.begin() + i * kMaxSize + kSubFrameLength,
+                buffer_.begin() + i * kMaxSize + kSubFrameLength + buffer_size_,
+                buffer_.begin() + i * kMaxSize);
+    }
+  }
+}
+
+}  // namespace webrtc