Changed queue implementation to the proposed vector-based solution.

webrtc/common_audio/swap_queue_unittest.cc

Index: webrtc/common_audio/swap_queue_unittest.cc
diff --git a/webrtc/common_audio/swap_queue_unittest.cc b/webrtc/common_audio/swap_queue_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..614f10d9cb50eeb68ec3f98a10b33aa4dded22c9
--- /dev/null
+++ b/webrtc/common_audio/swap_queue_unittest.cc
@@ -0,0 +1,239 @@
+/*
+ *  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 <algorithm>
+#include <map>
+#include <tuple>
+#include <vector>
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/common_audio/swap_queue.h"

[the sun, 2015/10/29 09:33:04]

Include swap_queue.h first, since we don't have any .cc file for it.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Ord...

[peah-webrtc, 2015/10/29 12:32:43]

Done.

+#include "webrtc/modules/interface/module_common_types.h"
+#include "webrtc/system_wrappers/interface/event_wrapper.h"
+#include "webrtc/system_wrappers/interface/sleep.h"

[the sun, 2015/10/29 09:33:04]

Clean away unused includes, please.

[peah-webrtc, 2015/10/29 12:32:42]

Done.

+#include "webrtc/system_wrappers/interface/thread_wrapper.h"
+
+namespace webrtc {
+
+namespace {
+
+// The type for a message.
+class SwapQueueTestMessage {

[the sun, 2015/10/29 09:33:04]

nit: just TestMessage
using the SwapQueue prefix makes it sound like it is actually tied to the
SwapQueue class, like SwapQueueItemVerifier

[peah-webrtc, 2015/10/29 12:32:43]

Good point! Renamed the class.

Done.

+ public:
+  SwapQueueTestMessage() {
+    id_ = -1;
+    info_ = -1;
+  }
+
+  SwapQueueTestMessage(const SwapQueueTestMessage& m)
+      : id_(m.id_), info_(m.info_) {}
+
+  SwapQueueTestMessage(int id, int info) {
+    id_ = id;
+    info_ = info;
+  }
+
+  bool Compare(const SwapQueueTestMessage& m) {

[the sun, 2015/10/29 09:33:04]

Total nit: This is C++, we can do "bool operator==(...)"

[peah-webrtc, 2015/10/29 12:32:42]

Good point!

Done.

+    return (id_ == m.id_ && info_ == m.info_);
+  }
+
+  void swap(SwapQueueTestMessage& m) {
+    std::swap(id_, m.id_);
+    std::swap(info_, m.info_);
+  }
+
+ private:
+  int id_;
+  int info_;
+};
+
+// Test parameter for the basic sample based SwapQueue Tests.
+const size_t kChunkSize = 3;
+
+// Queue item verification function for the vector test.
+bool LengthVerifierFunction(const std::vector<int>& v) {
+  RTC_CHECK(v.size() == kChunkSize);

[the sun, 2015/10/29 09:33:04]

Shouldn't use RTC_CHECK here. The verifier should just return true/false and
SwapQueue will act on the result.

[peah-webrtc, 2015/10/29 12:32:42]

Done.

+  return true;
+}
+
+// Queue item verifier for the vector test.
+class LengthVerifierFunctor {
+ public:
+  explicit LengthVerifierFunctor(size_t length) : length_(length) {}
+
+  bool operator()(const std::vector<int>& v) const {
+    return v.size() == length_;
+  }
+
+ private:
+  size_t length_;
+};
+
+// Queue item verifier for the FunctorVerification test.
+class IntVerifier {
+ public:
+  explicit IntVerifier(int threshold) : threshold_(threshold) {}
+
+  bool operator()(const int& i) const { return i > threshold_; }
+
+ private:
+  int threshold_;
+};
+
+}  // anonymous namespace
+
+TEST(SwapQueueTest, SuccessfulInvarianceVerification1) {

[the sun, 2015/10/29 09:33:04]

Avoid numbering the tests when you could let the name have some meaning:
"SuccessfulItemVerifyFunction".

ItemVerify, because ItemVerifier is the name we decided to use.

[the sun, 2015/10/29 09:33:04]

nit: could you please rearrange the tests to start with bread-and-butter stuff
(construct/destruct, insert, remove, full/empty, ...).

[peah-webrtc, 2015/10/29 12:32:42]

Done.

[peah-webrtc, 2015/10/29 12:32:43]

Done.

+  std::vector<int> template_element(kChunkSize);
+  SwapQueue<std::vector<int>,
+            SwapQueueItemVerifier<std::vector<int>, LengthVerifierFunction>>
+      queue(2, template_element);
+  std::vector<int> valid_chunk(kChunkSize, 0);
+
+  EXPECT_TRUE(queue.Insert(&valid_chunk));
+  EXPECT_EQ(valid_chunk.size(), kChunkSize);
+  EXPECT_TRUE(queue.Remove(&valid_chunk));
+  EXPECT_EQ(valid_chunk.size(), kChunkSize);
+}
+
+TEST(SwapQueueTest, SuccessfulInvarianceVerification2) {

[the sun, 2015/10/29 09:33:04]

nit: for consistency, "SuccessfulItemVerifyFunctor"

[peah-webrtc, 2015/10/29 12:32:42]

Done.

+  std::vector<int> template_element(kChunkSize);
+  LengthVerifierFunctor verifier(kChunkSize);
+  SwapQueue<std::vector<int>, LengthVerifierFunctor> queue(2, verifier,
+                                                           template_element);
+  std::vector<int> valid_chunk(kChunkSize, 0);
+
+  EXPECT_TRUE(queue.Insert(&valid_chunk));
+  EXPECT_EQ(valid_chunk.size(), kChunkSize);
+  EXPECT_TRUE(queue.Remove(&valid_chunk));
+  EXPECT_EQ(valid_chunk.size(), kChunkSize);
+}
+
+#if defined(GTEST_HAS_DEATH_TEST)

[the sun, 2015/10/29 09:33:05]

Karl use a different condition to enable death tests:
https://codereview.webrtc.org/1408403002/diff/240001/webrtc/base/array_view_u...

Not sure what is actually required. Can you guys figure out the correct way?

[peah-webrtc, 2015/10/29 12:32:42]

Done.

+TEST(SwapQueueTest, FunctorVerification) {

[the sun, 2015/10/29 09:33:04]

UnsuccessfulItemVerifyFunctor

[peah-webrtc, 2015/10/29 12:32:42]

Done.

+  IntVerifier verifier(-2);

[the sun, 2015/10/29 09:33:04]

IntVerifier is only used in this test - can you move the class in here instead?

[peah-webrtc, 2015/10/29 12:32:42]

Done.

+  SwapQueue<int, IntVerifier> queue(2, verifier);
+
+  int valid_value = 1;
+  int invalid_value = -4;
+  EXPECT_TRUE(queue.Insert(&valid_value));
+  EXPECT_TRUE(queue.Remove(&valid_value));
+  bool result;
+  EXPECT_DEATH(result = queue.Insert(&invalid_value), "");
+}
+
+TEST(SwapQueueTest, UnSuccessfulInvarianceVerification1) {

[the sun, 2015/10/29 09:33:04]

UnsuccessfulItemVerifyInsert

[peah-webrtc, 2015/10/29 12:32:43]

Done.

+  std::vector<int> template_element(kChunkSize);
+  SwapQueue<std::vector<int>,
+            SwapQueueItemVerifier<std::vector<int>, &LengthVerifierFunction>>
+      queue(2, template_element);
+  std::vector<int> invalid_chunk(kChunkSize - 1, 0);
+  bool result;
+  EXPECT_DEATH(result = queue.Insert(&invalid_chunk), "");
+}
+
+TEST(SwapQueueTest, UnSuccessfulInvarianceVerification2) {

[the sun, 2015/10/29 09:33:04]

UnsuccessfulItemVerifyRemove

[peah-webrtc, 2015/10/29 12:32:42]

Done.

+  std::vector<int> template_element(kChunkSize);
+  SwapQueue<std::vector<int>,
+            SwapQueueItemVerifier<std::vector<int>, &LengthVerifierFunction>>
+      queue(2, template_element);
+  std::vector<int> invalid_chunk(kChunkSize - 1, 0);
+  std::vector<int> valid_chunk(kChunkSize, 0);
+  EXPECT_TRUE(queue.Insert(&valid_chunk));
+  EXPECT_EQ(valid_chunk.size(), kChunkSize);
+  bool result;
+  EXPECT_DEATH(result = queue.Remove(&invalid_chunk), "");
+}
+#endif
+
+TEST(SwapQueueTest, MessageTest) {
+  const size_t kQueueSize = 200;
+  std::vector<SwapQueueTestMessage> messages;
+  SwapQueue<SwapQueueTestMessage> queue(kQueueSize);
+
+  for (size_t k = 0; k < kQueueSize; k++) {
+    messages.push_back(SwapQueueTestMessage(k % 7, k % 17));
+  }
+
+  for (size_t k = 0; k < kQueueSize; k++) {
+    SwapQueueTestMessage m(messages[k]);
+    EXPECT_TRUE(queue.Insert(&m));
+  }
+
+  for (size_t k = 0; k < kQueueSize; k++) {
+    SwapQueueTestMessage m;
+    EXPECT_TRUE(queue.Remove(&m));
+    EXPECT_TRUE(m.Compare(messages[k]));
+  }
+}
+
+TEST(SwapQueueTest, VectorTest) {
+  const size_t kQueueSize = 10;
+  const size_t kFrameLength = 160;
+  const size_t kDataLength = kQueueSize * kFrameLength;
+  std::vector<int16_t> buffer_reader(kFrameLength, 0);
+  std::vector<int16_t> buffer_writer(kFrameLength, 0);
+  std::vector<int16_t> template_queue_element(kFrameLength);

[the sun, 2015/10/29 09:33:04]

Just make this inline, in the queue instantiation:
SwapQueue<std::vector<int16_t>> queue(kQueueSize,
std::vector<int16_t>(kFrameLength));

[peah-webrtc, 2015/10/29 12:32:42]

Done.

+  SwapQueue<std::vector<int16_t>> queue(kQueueSize, template_queue_element);
+  std::vector<int16_t> samples(kDataLength);
+
+  for (size_t k = 0; k < kDataLength; k++) {
+    samples[k] = k % 9;
+  }
+
+  for (size_t k = 0; k < kQueueSize; k++) {
+    memcpy(&buffer_writer[0], &samples[k * kFrameLength],
+           kFrameLength * sizeof(samples[0]));
+    EXPECT_TRUE(queue.Insert(&buffer_writer));
+  }
+
+  for (size_t k = 0; k < kQueueSize; k++) {
+    EXPECT_TRUE(queue.Remove(&buffer_reader));
+
+    for (size_t j = 0; j < buffer_reader.size(); j++) {
+      EXPECT_EQ(buffer_reader[j], samples[k * kFrameLength + j]);
+    }
+  }
+}
+
+TEST(SwapQueueTest, FullQueue) {

[the sun, 2015/10/29 09:33:05]

It would be a good test to verify the Remove()d values for a queue that a) runs
full and b) we know will wrap the indices. Either add verification in this test,
or extend MessageTest or VectorTest to wrap around.

[peah-webrtc, 2015/10/29 12:32:42]

Done.

+  SwapQueue<int> queue(2);
+  int i = 0;
+  EXPECT_TRUE(queue.Insert(&i));
+  EXPECT_TRUE(queue.Insert(&i));
+  EXPECT_FALSE(queue.Insert(&i));
+  EXPECT_TRUE(queue.Remove(&i));
+  EXPECT_TRUE(queue.Insert(&i));
+  EXPECT_FALSE(queue.Insert(&i));
+}
+
+TEST(SwapQueueTest, EmptyQueue) {
+  SwapQueue<int> queue(2);
+  int i = 0;
+  EXPECT_FALSE(queue.Remove(&i));
+  EXPECT_TRUE(queue.Insert(&i));
+  EXPECT_TRUE(queue.Remove(&i));
+  EXPECT_FALSE(queue.Remove(&i));
+}
+
+TEST(SwapQueueTest, InitializeTest) {
+  std::vector<int> i(kChunkSize, 0);
+  SwapQueue<std::vector<int>> queue(2, i);
+
+  EXPECT_TRUE(queue.Insert(&i));
+  EXPECT_EQ(i.size(), kChunkSize);
+  EXPECT_TRUE(queue.Insert(&i));
+  EXPECT_EQ(i.size(), kChunkSize);
+  EXPECT_TRUE(queue.Remove(&i));
+  EXPECT_EQ(i.size(), kChunkSize);
+  EXPECT_TRUE(queue.Remove(&i));
+  EXPECT_EQ(i.size(), kChunkSize);
+}
+

[the sun, 2015/10/29 09:33:04]

I didn't see a test for Clear().

[peah-webrtc, 2015/10/29 12:32:42]

Added another test case for that.

Done.

+}  // namespace webrtc