Passing frame decoded callback to the codec

media/cast/test/encode_decode_test.cc

Index: media/cast/test/encode_decode_test.cc
diff --git a/media/cast/test/encode_decode_test.cc b/media/cast/test/encode_decode_test.cc
index bc2c0a17256fa2e172d1c9c5a36807f33e0eb8ca..2a5d68cb6055eab8ecf99a93df43649f9d62b4be 100644
--- a/media/cast/test/encode_decode_test.cc
+++ b/media/cast/test/encode_decode_test.cc
@@ -8,7 +8,10 @@
 
 #include <gtest/gtest.h>
 
+#include "base/bind.h"
 #include "base/memory/scoped_ptr.h"
+#include "media/cast/cast_environment.h"
+#include "media/cast/test/fake_task_runner.h"
 #include "media/cast/test/video_utility.h"
 #include "media/cast/video_receiver/codecs/vp8/vp8_decoder.h"
 #include "media/cast/video_sender/codecs/vp8/vp8_encoder.h"
@@ -16,18 +19,65 @@
 namespace media {
 namespace cast {
 
-namespace {
+static const int64 kStartMillisecond = GG_INT64_C(1245);

[Alpha Left Google, 2013/11/06 02:22:06]

The other const values don't have static, why this one needs it?

[mikhal, 2013/11/06 18:29:16]

The are all local to this file - making them all static (aligning with the rest
of the Cast code).
On 2013/11/06 02:22:06, Alpha wrote:

 const int kWidth = 1280;
 const int kHeight = 720;
 const int kStartbitrate = 4000000;
 const int kMaxQp = 54;
 const int kMinQp = 4;
 const int kMaxFrameRate = 30;
-}  // namespace
+
+class EncodeDecodeTestFrameCallback :

[Alpha Left Google, 2013/11/06 02:22:06]

nit: put this in an anonymous namespace. 

[mikhal, 2013/11/06 18:29:16]

Done.

+  public base::RefCountedThreadSafe<EncodeDecodeTestFrameCallback> {

[Alpha Left Google, 2013/11/06 02:22:06]

nit: I think this is 4 space indentation.

[mikhal, 2013/11/06 18:29:16]

Done.

+ public:
+  EncodeDecodeTestFrameCallback()
+      : num_called_(0),
+        original_frame_() {

[Alpha Left Google, 2013/11/06 02:22:06]

nit: This line is not needed.

[mikhal, 2013/11/06 18:29:16]

Done.

+    original_frame_.width = kWidth;
+    original_frame_.height = kHeight;
+  }
+
+  void SetFrameStartValue(int start_value) {
+    PopulateVideoFrame(&original_frame_, start_value);
+  }
+
+  void DecodeComplete(scoped_ptr<I420VideoFrame> decoded_frame,
+                      const base::TimeTicks& render_time) {
+    ++num_called_;
+    // Compare frames.
+    // Compare resolution.
+    EXPECT_EQ(original_frame_.width, decoded_frame->width);
+    EXPECT_EQ(original_frame_.height, decoded_frame->height);
+    // Compare data.
+    EXPECT_GT(I420PSNR(original_frame_, *(decoded_frame.get())), 40.0);
+  }
+
+  int num_called() {

[Alpha Left Google, 2013/11/06 02:22:06]

nit: int num_called() const {}

[mikhal, 2013/11/06 18:29:16]

Done.

+    return num_called_;
+  }
+
+ protected:
+  virtual ~EncodeDecodeTestFrameCallback() {}
+
+ private:
+  friend class base::RefCountedThreadSafe<EncodeDecodeTestFrameCallback>;
+
+  int num_called_;
+  I420VideoFrame original_frame_;
+

[Alpha Left Google, 2013/11/06 02:22:06]

nit: remove this empty line.

[mikhal, 2013/11/06 18:29:16]

Done.

+};
 
 class EncodeDecodeTest : public ::testing::Test {
  protected:
-  EncodeDecodeTest() {
+  EncodeDecodeTest()
+      : task_runner_(new test::FakeTaskRunner(&testing_clock_)),
+        // CastEnvironment will only be used by the vp8 decoder; Enable only the
+        // video decoder and main threads.
+        cast_environment_(new CastEnvironment(&testing_clock_, task_runner_,
+            NULL, NULL, NULL, task_runner_)),
+        test_callback_(new EncodeDecodeTestFrameCallback()) {
+    testing_clock_.Advance(
+        base::TimeDelta::FromMilliseconds(kStartMillisecond));
     encoder_config_.max_number_of_video_buffers_used = 1;
     encoder_config_.number_of_cores = 1;
     encoder_config_.width  = kWidth;
@@ -39,9 +89,11 @@ class EncodeDecodeTest : public ::testing::Test {
     int max_unacked_frames = 1;
     encoder_.reset(new Vp8Encoder(encoder_config_, max_unacked_frames));
     // Initialize to use one core.
-    decoder_.reset(new Vp8Decoder(1));
+    decoder_.reset(new Vp8Decoder(1, cast_environment_));
   }
 
+  virtual ~EncodeDecodeTest() {}
+
   virtual void SetUp() {

[Alpha Left Google, 2013/11/06 02:22:06]

nit: while you're at it. This should SetUp() OVERRIDE.

[mikhal, 2013/11/06 18:29:16]

Done.

     // Create test frame.
     int start_value = 10;  // Random value to start from.
@@ -49,6 +101,7 @@ class EncodeDecodeTest : public ::testing::Test {
     video_frame_->width = encoder_config_.width;
     video_frame_->height = encoder_config_.height;
     PopulateVideoFrame(video_frame_.get(), start_value);
+    test_callback_->SetFrameStartValue(start_value);
   }
 
   virtual void TearDown() {

[Alpha Left Google, 2013/11/06 02:22:06]

nit: while you're at it. This should TearDown() OVERRIDE.

[mikhal, 2013/11/06 18:29:16]

Done.

@@ -57,31 +110,28 @@ class EncodeDecodeTest : public ::testing::Test {
     delete [] video_frame_->v_plane.data;
   }
 
-  void Compare(const I420VideoFrame& original_image,
-               const I420VideoFrame& decoded_image) {
-    // Compare resolution.
-    EXPECT_EQ(original_image.width, decoded_image.width);
-    EXPECT_EQ(original_image.height, decoded_image.height);
-    // Compare data.
-    EXPECT_GT(I420PSNR(original_image, decoded_image), 40.0);
-  }
-
   VideoSenderConfig encoder_config_;
   scoped_ptr<Vp8Encoder> encoder_;
   scoped_ptr<Vp8Decoder> decoder_;
   scoped_ptr<I420VideoFrame> video_frame_;
+  base::SimpleTestTickClock testing_clock_;
+  scoped_refptr<test::FakeTaskRunner> task_runner_;
+  scoped_refptr<CastEnvironment> cast_environment_;
+  scoped_refptr<EncodeDecodeTestFrameCallback> test_callback_;
 };
 
 TEST_F(EncodeDecodeTest, BasicEncodeDecode) {
   EncodedVideoFrame encoded_frame;
-  I420VideoFrame decoded_frame;
   // Encode frame.
   encoder_->Encode(*(video_frame_.get()), &encoded_frame);
   EXPECT_GT(encoded_frame.data.size(), GG_UINT64_C(0));
   // Decode frame.
-  decoder_->Decode(encoded_frame, &decoded_frame);
-  // Validate data.
-  Compare(*(video_frame_.get()), decoded_frame);
+  base::TimeTicks render_time;

[Alpha Left Google, 2013/11/06 02:22:06]

nit: There's no need to have this local variable.

[mikhal, 2013/11/06 18:29:16]

Done.

+  VideoFrameDecodedCallback frame_decoded_callback =

[Alpha Left Google, 2013/11/06 02:22:06]

nit: We don't usually define the callback as a local variable unless it is
necessary. For this case you can just move this to Decode().

[mikhal, 2013/11/06 18:29:16]

Done.

+      base::Bind(&EncodeDecodeTestFrameCallback::DecodeComplete,
+                 test_callback_);
+  decoder_->Decode(&encoded_frame, render_time, frame_decoded_callback);

[Alpha Left Google, 2013/11/06 02:22:06]

You can replace render_time with base::TimeTicks(). Also define the callback
inline.

[mikhal, 2013/11/06 18:29:16]

Done.

+  task_runner_->RunTasks();
 }
 
 }  // namespace cast