Bitexactness test for the highpass filter

webrtc/modules/audio_processing/test/vector_based_audio_frame.cc

+/*
+ *  Copyright (c) 2015 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/test/vector_based_audio_frame.h"
+
+namespace webrtc {
+
+VectorBasedAudioFrame::VectorBasedAudioFrame(size_t frame_length,
+                                             size_t num_channels,
+                                             const float* initial_values)
+    : VectorBasedAudioFrame(frame_length, num_channels) {
+  for (size_t ch = 0; ch < num_channels; ++ch) {

[hlundin-webrtc, 2015/12/10 12:12:17]

Do one memcpy for all of the data:
memcpy(&frame_[0][0], initial_values, frame_length * num_channels);

[peah-webrtc, 2015/12/22 06:28:10]

True. This is now done in the new code.

Acknowledged.

+    memcpy(&frame_[ch][0], &initial_values[ch * frame_length],
+           frame_length_ * sizeof(initial_values[0]));
+  }
+}
+
+VectorBasedAudioFrame::VectorBasedAudioFrame(size_t frame_length,
+                                             size_t num_channels)
+    : frame_length_(frame_length), num_channels_(num_channels) {
+  channels_.resize(num_channels * frame_length_);

[hlundin-webrtc, 2015/12/10 12:12:17]

num_channels_
- or -
frame_length

[peah-webrtc, 2015/12/22 06:28:10]

Good catch!
Acknowledged.

+  frame_.resize(num_channels);
+  for (size_t ch = 0; ch < num_channels; ++ch) {
+    frame_[ch] = &channels_[ch * frame_length_];
+  }
+}
+
+void VectorBasedAudioFrame::Randomize(test::Random* rand_gen_) const {

[hlundin-webrtc, 2015/12/10 12:12:17]

How can this be const when you are in fact modifying frame_?

[peah-webrtc, 2015/12/22 06:28:10]

True! It is really strange that that even compiled!

The code is, however, now replaced with a better version.

Acknowledged.

+  for (size_t ch = 0; ch < num_channels_; ++ch) {

[hlundin-webrtc, 2015/12/10 12:12:17]

Do one grand for-loop. Make it range-based over frame_.
for (auto& value : frame_) {
  value = static_cast<float>(rand_gen_->Rand(0, 32767 + 32768) - 32768) /
32768.0f;
}

[peah-webrtc, 2015/12/22 06:28:10]

Acknowledged.

+    for (size_t k = 0; k < frame_length_; ++k) {
+      frame_[ch][k] =
+          static_cast<float>(rand_gen_->Rand(0, 32767 + 32768) - 32768) /

[the sun, 2015/12/10 12:03:53]

For testing the HPF, I'd also use an impulse.

[peah-webrtc, 2015/12/22 06:28:10]

Why would that be better than using WGN signal for testing the bitexactness?
They should both excite all aspects of the filter.

+          32768.0f;
+    }
+  }
+}
+
+void VectorBasedAudioFrame::CopyToAudioBuffer(StreamConfig stream_config,
+                                              AudioBuffer* buffer) {
+  buffer->CopyFrom(&frame_[0], stream_config);
+}
+
+void VectorBasedAudioFrame::CopyFromAudioBuffer(StreamConfig stream_config,
+                                                AudioBuffer* buffer) {

[hlundin-webrtc, 2015/12/10 12:12:17]

I would like buffer to be const, but I guess that doesn't work since
AudioBuffer::CopyTo isn't const declared.

[peah-webrtc, 2015/12/22 06:28:10]

True. That is the reason for this. And the reason that is not const declared is
that it may cache the internal conversion as well during this operation.

+  buffer->CopyTo(stream_config, &frame_[0]);
+}
+
+bool VectorBasedAudioFrame::CompareTo(VectorBasedAudioFrame* other_frame,
+                                      float tolerance) const {
+  if (num_channels_ != other_frame->num_channels()) {

[hlundin-webrtc, 2015/12/10 12:12:17]

Won't you have to check the frame_lengths too? Or, see my suggestion for
total_length() below.

[peah-webrtc, 2015/12/22 06:28:10]

Agree. This is now replaced with another function.

Acknowledged.

+    return false;
+  }
+
+  const std::vector<float*>* const other_frame_array = other_frame->get_frame();
+  for (size_t ch = 0; ch < num_channels_; ++ch) {

[hlundin-webrtc, 2015/12/10 12:12:17]

Can you rewrite this to one grand for loop:
for (size_t i = 0; i < total_length(); ++i) {
  if (fabs(frame_[i] - (*other_frame_array)[i]) > tolerance)
    return false;
}

I also suggest you add a convenience getter
size_t VectorBasedAudioFrame::total_length() const {
  return num_channels_ * frame_length_;
}

[peah-webrtc, 2015/12/22 06:28:10]

Agree. that would be better. This is now replaced by another function.
Acknowledged.

+    for (size_t k = 0; k < other_frame->frame_length(); ++k) {
+      if (fabs(frame_[ch][k] - (*other_frame_array)[ch][k]) > tolerance) {

[the sun, 2015/12/10 12:03:54]

In gtest there's EXPECT_NEAR also.

[peah-webrtc, 2015/12/22 06:28:10]

True, but since I now use EXPECT_PRED_FORMAT2() where I want to also catch the
error in order to make a proper printout and flag that an error has occurred, I
cannot see how I can use EXPECT_NEAR in any good way. Do you have any suggestion
for that?

+        return false;
+      }
+    }
+  }
+  return true;
+}
+
+void VectorBasedAudioFrame::PrintValues() {
+  PrintValues(frame_length_);
+}
+
+void VectorBasedAudioFrame::PrintValues(size_t num_samples_per_channel) {
+  printf("{");
+  for (size_t ch = 0; ch < num_channels_; ++ch) {
+    printf("{");
+    for (size_t k = 0; k < num_samples_per_channel; ++k) {
+      printf("%ff", frame_[ch][k]);
+      if (k < (num_samples_per_channel - 1)) {
+        printf(", ");
+      }
+    }
+    printf("}");
+    if (ch < (num_channels_ - 1)) {
+      printf(", ");
+    }
+  }
+  printf("}");
+}
+
+}  // namespace webrtc