FrameBuffer for the new jitter buffer.

webrtc/modules/video_coding/frame_buffer2_unittest.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 <algorithm>
+#include <cstring>
+#include <limits>
+#include <vector>
+
+#include "webrtc/modules/video_coding/frame_buffer2.h"

[danilchap, 2016/05/13 13:31:04]

put this include before c++ includes

+
+#include "webrtc/base/random.h"
+#include "webrtc/base/platform_thread.h"
+#include "webrtc/base/thread.h"

[danilchap, 2016/05/13 13:31:04]

base/thread.h is not in rtc_base_approved - do not include it in modules/
(specially that you do not use it: same kForever declared in the Event that make
more sense to use since you are waiting for Events)

[philipel, 2016/05/16 12:10:22]

Done.

+#include "testing/gmock/include/gmock/gmock.h"

[danilchap, 2016/05/13 13:31:04]

alphabetical order (testing before webrtc)

[philipel, 2016/05/16 12:10:22]

Done.

+#include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/modules/video_coding/timing.h"
+#include "webrtc/modules/video_coding/jitter_estimator.h"
+
+namespace webrtc {
+namespace video_coding {
+
+class VCMTimingMock : public VCMTiming {

[danilchap, 2016/05/13 13:31:04]

this object look like Fake, not Mock

[philipel, 2016/05/16 12:10:22]

Done.

+ public:
+  explicit VCMTimingMock(Clock* clock) : VCMTiming(clock) {}
+
+  int64_t
+  RenderTimeMs(uint32_t frame_timestamp, int64_t now_ms) const override {
+    if (last_ms_ == -1) {
+      last_ms_ = now_ms + kDelayMs;
+      last_timestamp_ = frame_timestamp;
+    }
+
+    uint32_t diff = MinDiff(frame_timestamp, last_timestamp_);
+    if (AheadOf(frame_timestamp, last_timestamp_))
+      last_ms_ += diff / 90;
+    else
+      last_ms_ -= diff / 90;
+
+    last_timestamp_ = frame_timestamp;
+    return last_ms_;
+  }
+
+
+  uint32_t
+  MaxWaitingTime(int64_t render_time_ms, int64_t now_ms) const override {
+    return std::max<int>(0, render_time_ms - now_ms - kDecodeTime);
+  }
+

[danilchap, 2016/05/13 13:31:04]

private:

[philipel, 2016/05/16 12:10:21]

Done.

+  static const int kDelayMs = 50;

[danilchap, 2016/05/13 13:31:04]

constexpr

[philipel, 2016/05/16 12:10:22]

Done.

+  static const int kDecodeTime = kDelayMs/2;
+  mutable uint32_t last_timestamp_ = 0;
+  mutable int64_t last_ms_ = -1;
+};
+
+class VCMJitterEstimatorMock : public VCMJitterEstimator {
+ public:
+  explicit VCMJitterEstimatorMock(Clock* clock) : VCMJitterEstimator(clock) {}
+
+  MOCK_METHOD1(UpdateRtt, void(int64_t rttMs));
+  MOCK_METHOD3(UpdateEstimate, void(int64_t frameDelayMs,
+                                    uint32_t frameSizeBytes,
+                                    bool incompleteFrame));
+};
+
+class FrameObjectMock : public FrameObject {
+ public:
+  MOCK_CONST_METHOD1(GetBitstream, bool(uint8_t* destination));
+};
+
+class TestFrameBuffer2 : public ::testing::Test {
+ protected:
+  TestFrameBuffer2() :
+      clock_(0),
+      timing_(&clock_),
+      jitter_estimator_(&clock_),
+      buffer_(&clock_, &jitter_estimator_, &timing_),
+      rand_(0x34678213),
+      tear_down_(false),
+      extract_thread_(&ExtractLoop, this, "Extract Thread"),
+      trigger_extract_event_(false, false),
+      crit_acquired_event_(false, false) {}
+
+  void SetUp() override {
+    extract_thread_.Start();
+  }
+
+  void TearDown() override {
+    tear_down_ = true;
+    trigger_extract_event_.Set();
+    extract_thread_.Stop();
+  }
+
+  template <typename... T>

[danilchap, 2016/05/13 13:31:04]

do you really want to use variable number of parameters with variable types?
May be instead you want to take rtc::ArrayView<uint16_t> as last parameter.

While it make InsertFrame calls two-lines instead of one, it might be clear:
const uint16_t kReferences2[] = {pid + i, pid + i - 1};
InsertFrame(pid + i + 1, 0, ts_tl0 + kFps20, false, kReferences2);

[philipel, 2016/05/16 12:10:22]

Prefer variadic function.

+  void InsertFrame(uint16_t picture_id,
+                   uint8_t spatial_layer,
+                   int64_t ts_ms,
+                   bool inter_layer_predicted,
+                   T... refs) {
+    static_assert(sizeof...(refs) <= kMaxReferences,
+                  "To many references specified for FrameObject.");
+    std::array<uint16_t, sizeof...(refs)> references = {{refs...}};
+
+    std::unique_ptr<FrameObjectMock> frame(new FrameObjectMock());
+    frame->picture_id = picture_id;
+    frame->spatial_layer = spatial_layer;
+    frame->timestamp = ts_ms * 90;
+    frame->num_references = references.size();
+    for (size_t r = 0; r < references.size(); ++r)
+      frame->references[r] = references[r];
+
+    buffer_.InsertFrame(std::move(frame));
+  }
+
+  void ExtractFrame(int64_t max_wait_time = 0) {
+    crit_.Enter();

[danilchap, 2016/05/13 13:31:04]

do you need to check max_wait_time while in crit_? Guess not.
You can then enter/leave crit_ with CritScope (and extra {} in else branch)

[philipel, 2016/05/16 12:10:21]

We always want to hold |crit_| in the beginning of this function in order to
sync with other events.

+    if (max_wait_time == 0) {
+      frames_.emplace_back(buffer_.NextFrame(0));
+      crit_.Leave();
+    } else {
+      max_wait_time_ = max_wait_time;
+      trigger_extract_event_.Set();
+      crit_.Leave();
+      // Make sure |crit_| is aquired by |extract_thread_| before returning.
+      crit_acquired_event_.Wait(rtc::Thread::kForever);

[danilchap, 2016/05/13 13:31:05]

Wait(rtc::Event::kForever)

[philipel, 2016/05/16 12:10:22]

Done.

+    }
+  }
+
+  void CheckFrame(size_t index,

[danilchap, 2016/05/13 13:31:04]

look like you want two functions: CheckNoFrame with with one parameter, and
CheckFrame with three. Instead of a branch inside the test.

[danilchap, 2016/05/17 12:35:01]

overlooked or disagreed?

[philipel, 2016/05/17 14:57:04]

Fixed now :)

+                  int picture_id = -1,
+                  int spatial_layer = -1) {
+    rtc::CritScope lock(&crit_);
+    if (frames_.size() <= index) {

[danilchap, 2016/05/13 13:31:05]

ASSERT_LT(index, frames_.size()) << "Error message";

[philipel, 2016/05/16 12:10:22]

Done.

+      ADD_FAILURE() << "Expected frame at index " << index
+                    << " but vector does not contain element at index "
+                    << index;
+      return;
+    }
+
+
+    if (picture_id == -1) {
+      ASSERT_EQ(nullptr, frames_[index].get());

[danilchap, 2016/05/13 13:31:05]

ASSERT_FALSE(frames_[index]) will do the same, but better match assert in else
branch

[philipel, 2016/05/16 12:10:22]

Done.

+    } else {
+      ASSERT_TRUE(frames_[index]);
+      ASSERT_EQ(picture_id, frames_[index]->picture_id);
+      ASSERT_EQ(spatial_layer, frames_[index]->spatial_layer);
+    }
+  }
+
+  static bool ExtractLoop(void* obj) {

[danilchap, 2016/05/13 13:31:05]

may be create non-static version bool ExtractLoop();
and call that one from this static function.
implementation would look nice then.

[philipel, 2016/05/16 12:10:21]

Acknowledged.

+    TestFrameBuffer2* tfb = static_cast<TestFrameBuffer2*>(obj);
+    while (true) {
+      tfb->trigger_extract_event_.Wait(rtc::Thread::kForever);
+      tfb->crit_.Enter();

[danilchap, 2016/05/13 13:31:05]

rtc::CritScope lock(&tfb->crit_);

[philipel, 2016/05/16 12:10:21]

Done.

+      tfb->crit_acquired_event_.Set();
+      if (tfb->tear_down_) {
+        tfb->crit_.Leave();
+        return false;
+      }
+
+      tfb->frames_.emplace_back(tfb->buffer_.NextFrame(tfb->max_wait_time_));
+      tfb->crit_.Leave();
+    }
+  }
+
+  uint16_t Rand() { return rand_.Rand(std::numeric_limits<uint32_t>::max()); }

[danilchap, 2016/05/13 13:31:05]

uint16_t or uint32?

btw, you can run rand_.Rand<uint32_t>(); instead of explicitly specifying max.

[philipel, 2016/05/16 12:10:22]

Done.

+
+  static const int kMaxReferences = 5;

[danilchap, 2016/05/13 13:31:05]

constexpr and may be move above functions

[philipel, 2016/05/16 12:10:22]

Done.

+  static const int kFps1 = 1000;
+  static const int kFps10 = kFps1/10;
+  static const int kFps20 = kFps1/20;
+
+  SimulatedClock clock_;
+  VCMTimingMock timing_;
+  VCMJitterEstimatorMock jitter_estimator_;
+  FrameBuffer buffer_;
+  std::vector<std::unique_ptr<FrameObject>> frames_;
+  Random rand_;
+
+  int64_t max_wait_time_;
+  bool tear_down_;
+  rtc::PlatformThread extract_thread_;
+  rtc::Event trigger_extract_event_;
+  rtc::Event crit_acquired_event_;
+  rtc::CriticalSection crit_;
+};
+
+TEST_F(TestFrameBuffer2, ExtractFromEmptyBuffer) {
+  ExtractFrame();
+  CheckFrame(0);
+}
+
+TEST_F(TestFrameBuffer2, WaitForFrame) {
+  uint16_t pid = Rand();
+  uint32_t ts = Rand();
+
+  ExtractFrame(20);
+  InsertFrame(pid, 0, ts, false);
+  CheckFrame(0, pid, 0);
+}
+
+TEST_F(TestFrameBuffer2, OneSuperFrame) {
+  uint16_t pid = Rand();
+  uint32_t ts = Rand();
+
+  ExtractFrame(20);
+  InsertFrame(pid, 1, ts, false);
+  InsertFrame(pid, 0, ts, false);
+  ExtractFrame();
+
+  CheckFrame(0, pid, 0);
+  CheckFrame(1, pid, 1);
+}
+
+TEST_F(TestFrameBuffer2, OneLayerStream) {
+  uint16_t pid = Rand();
+  uint32_t ts = Rand();
+
+  InsertFrame(pid, 0, ts, false);
+  ExtractFrame();
+  CheckFrame(0, pid, 0);
+  for (int i = 1; i < 10; ++i) {
+    InsertFrame(pid + i, 0, ts + i * kFps10, false, pid + i - 1);
+    ExtractFrame();
+    clock_.AdvanceTimeMilliseconds(kFps10);
+    CheckFrame(i, pid + i, 0);
+  }
+}
+
+TEST_F(TestFrameBuffer2, OneLayerStreamReordered) {
+  uint16_t pid = Rand();
+  uint32_t ts = Rand();
+
+  InsertFrame(pid, 0, ts, false);
+  ExtractFrame();
+  CheckFrame(0, pid, 0);
+  for (int i = 1; i < 10; i += 2) {
+    ExtractFrame(15);
+    InsertFrame(pid + i + 1, 0, ts + (i + 1) * kFps10, false, pid + i);
+    clock_.AdvanceTimeMilliseconds(kFps10);
+    InsertFrame(pid + i    , 0, ts +       i * kFps10, false, pid + i - 1);
+    clock_.AdvanceTimeMilliseconds(kFps10);
+    ExtractFrame();
+    CheckFrame(i    , pid + i, 0);
+    CheckFrame(i + 1, pid + i + 1, 0);
+  }
+}
+
+TEST_F(TestFrameBuffer2, DropTemporalLayerSlowDecoder) {
+  uint16_t pid = Rand();
+  uint32_t ts = Rand();
+
+  InsertFrame(pid, 0, ts, false);
+  InsertFrame(pid + 1, 0, ts + kFps20, false);
+  for (int i = 2; i < 10; i += 2) {
+    uint32_t ts_tl0 = ts + i/2 * kFps10;
+    InsertFrame(pid + i    , 0, ts_tl0         , false, pid + i - 2);
+    InsertFrame(pid + i + 1, 0, ts_tl0 + kFps20, false, pid + i, pid + i - 1);
+  }
+
+  for (int i = 0; i < 10; ++i) {
+    ExtractFrame();
+    clock_.AdvanceTimeMilliseconds(60);
+  }
+
+  CheckFrame(0, pid    , 0);
+  CheckFrame(1, pid + 1, 0);
+  CheckFrame(2, pid + 2, 0);
+  CheckFrame(3, pid + 4, 0);
+  CheckFrame(4, pid + 6, 0);
+  CheckFrame(5, pid + 8, 0);
+  CheckFrame(6);
+  CheckFrame(7);
+  CheckFrame(8);
+  CheckFrame(9);
+}
+
+TEST_F(TestFrameBuffer2, DropSpatialLayerSlowDecoder) {
+  uint16_t pid = Rand();
+  uint32_t ts = Rand();
+
+  InsertFrame(pid, 0, ts, false);
+  InsertFrame(pid, 1, ts, false);
+  for (int i = 1; i < 6; ++i) {
+    uint32_t ts_tl0 = ts + i * kFps10;
+    InsertFrame(pid + i, 0, ts_tl0, false, pid + i - 1);
+    InsertFrame(pid + i, 1, ts_tl0, false, pid + i - 1);
+  }
+
+  ExtractFrame();
+  ExtractFrame();
+  clock_.AdvanceTimeMilliseconds(55);
+  for (int i = 2; i < 12; ++i) {
+    ExtractFrame();
+    clock_.AdvanceTimeMilliseconds(55);
+  }
+
+  CheckFrame(0, pid    , 0);
+  CheckFrame(1, pid    , 1);
+  CheckFrame(2, pid + 1, 0);
+  CheckFrame(3, pid + 1, 1);
+  CheckFrame(4, pid + 2, 0);
+  CheckFrame(5, pid + 2, 1);
+  CheckFrame(6, pid + 3, 0);
+  CheckFrame(7, pid + 4, 0);
+  CheckFrame(8, pid + 5, 0);
+  CheckFrame(9);
+  CheckFrame(10);
+  CheckFrame(11);
+}
+
+}  // namespace video_coding
+}  // namespace webrtc