Readability review.

remoting/host/video_frame_recorder_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/video_frame_recorder.h"
 
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "remoting/codec/video_encoder_verbatim.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_region.h"
 
 namespace webrtc {
 
 // Define equality operator for DesktopFrame to allow use of EXPECT_EQ().
 static bool operator==(const DesktopFrame& a,
                        const DesktopFrame& b) {
   if ((a.size().equals(b.size())) &&
       (a.updated_region().Equals(b.updated_region())) &&
       (a.dpi().equals(b.dpi()))) {

[Peter Kasting, 2014/08/22 06:38:31]

Nit: Reversing this conditional takes no more LOC total but avoids one level of
indenting for the remaining code.

[Wez, 2014/08/25 23:23:13]

Done.

     for (int i = 0; i < a.size().height(); ++i) {
       if (memcmp(a.data() + a.stride() * i,
                  b.data() + b.stride() * i,
                  a.size().width() * DesktopFrame::kBytesPerPixel) != 0) {
         return false;
       }
     }
     return true;
   }
   return false;
 }
 
 } // namespace
 
 namespace remoting {
 
 const int64_t kMaxContentBytes = 10 * 1024 * 1024;

[Peter Kasting, 2014/08/22 06:38:31]

Nit: Again, define these in the most local scopes possible.

[Wez, 2014/08/25 23:23:13]

Done.

 const int kWidth = 640;
 const int kHeight = 480;
 const int kTestFrameCount = 6;
 
 class VideoFrameRecorderTest : public testing::Test {
  public:
   VideoFrameRecorderTest();
 
   virtual void SetUp() OVERRIDE;
   virtual void TearDown() OVERRIDE;
 
   void CreateAndWrapEncoder();

[Peter Kasting, 2014/08/22 06:38:30]

Comments?

[Wez, 2014/08/25 23:23:13]

Done.

   scoped_ptr<webrtc::DesktopFrame> CreateNextFrame();
   void CreateTestFrames();
   void EncodeTestFrames();
   void EncodeDummyFrame();
   void StartRecording();
   void VerifyTestFrames();
 
  protected:

[Peter Kasting, 2014/08/22 06:38:31]

Nit: Again, try to avoid protected members if possible.

[Wez, 2014/08/25 23:23:13]

Again, since this is a test base, protected feels more appropriate, but let me
know if you disagree.

   base::MessageLoop message_loop_;
 
   scoped_ptr<VideoFrameRecorder> recorder_;
   scoped_ptr<VideoEncoder> encoder_;
 
   std::list<webrtc::DesktopFrame*> test_frames_;

[Peter Kasting, 2014/08/22 06:38:31]

Nit: Consider a typedef for this so you can use it in loops below

[Wez, 2014/08/25 23:23:13]

Done.

   int frame_count_;
 };

[Peter Kasting, 2014/08/22 06:38:31]

Nit: DISALLOW_COPY_AND_ASSIGN

 
 VideoFrameRecorderTest::VideoFrameRecorderTest() : frame_count_(0) {}
 
 void VideoFrameRecorderTest::SetUp() {
   recorder_.reset(new VideoFrameRecorder());
   recorder_->SetMaxContentBytes(kMaxContentBytes);
 }
 
 void VideoFrameRecorderTest::TearDown() {
   ASSERT_TRUE(test_frames_.empty());
@@ skipping 25 matching lines @@
   memset(frame->data(), frame_count_, frame->stride() * kHeight);
   frame->set_dpi(webrtc::DesktopVector(frame_count_, frame_count_));
   frame->mutable_updated_region()->SetRect(
       webrtc::DesktopRect::MakeWH(frame_count_, frame_count_));
   ++frame_count_;
 
   return frame.Pass();
 }
 
 void VideoFrameRecorderTest::CreateTestFrames() {
   for (int i=0; i < kTestFrameCount; ++i) {

[Peter Kasting, 2014/08/22 06:38:31]

Nit: Spaces around '='

Super-tiny nit: Since it's a count of objects in memory, the constant and loop
index here should probably be size_t rather than int.

[Wez, 2014/08/25 23:23:12]

Done, although I tend to prefer int over size_t on the basis of size_t
potentially falling foul of unsigned int gotchas.

     test_frames_.push_back(CreateNextFrame().release());
   }
 }
 
 void VideoFrameRecorderTest::EncodeTestFrames() {
   std::list<webrtc::DesktopFrame*>::iterator i;

[Peter Kasting, 2014/08/22 06:38:30]

Nit: Declare iterators in the loop declaration unless they're needed outside it

[Wez, 2014/08/25 23:23:12]

Done.

   for (i = test_frames_.begin(); i != test_frames_.end(); ++i) {
     scoped_ptr<VideoPacket> packet = encoder_->Encode(*(*i));

[Peter Kasting, 2014/08/22 06:38:31]

Nit: "scoped_ptr<VideoPacket> packet = " unnecessary?  (2 places)

[Wez, 2014/08/25 23:23:13]

Done.

In theory Encode() could return a NULL pointer, but never actually should, so
I've added ASSERT_TRUE() around these two calls.

 
     // Process tasks to let the recorder pick up the frame.
     base::RunLoop().RunUntilIdle();
   }
 }
 
 void VideoFrameRecorderTest::EncodeDummyFrame() {
   webrtc::BasicDesktopFrame dummy_frame(webrtc::DesktopSize(kWidth, kHeight));
   scoped_ptr<VideoPacket> packet = encoder_->Encode(dummy_frame);
   base::RunLoop().RunUntilIdle();
@@ skipping 18 matching lines @@
   }
 
   EXPECT_FALSE(recorder_->NextFrame());
 }
 
 // Basic test that creating & tearing down VideoFrameRecorder doesn't crash.
 TEST_F(VideoFrameRecorderTest, CreateDestroy) {
 }
 
 // Basic test that creating, starting, stopping and destroying a
 // VideoFrameRecorder don't end the world.

[Peter Kasting, 2014/08/22 06:38:31]

Nit: don't end the world -> doesn't crash (or some other more precise, less
colloquial phrase)

[Wez, 2014/08/25 23:23:12]

Done.

 TEST_F(VideoFrameRecorderTest, StartStop) {
   StartRecording();
   recorder_->SetEnableRecording(false);
 }
 
 // Test that tearing down the VideoFrameRecorder while the VideoEncoder
 // wrapper exists doesn't crash.
 TEST_F(VideoFrameRecorderTest, DestroyVideoFrameRecorderFirst) {
   CreateAndWrapEncoder();
 
@@ skipping 39 matching lines @@
   // Verify that the recorded frames match the ones passed to the encoder.
   VerifyTestFrames();
 }
 
 // Test that when asked to record more frames than the maximum content bytes
 // limit allows, the first encoded frames are dropped.
 TEST_F(VideoFrameRecorderTest, MaxContentBytesEnforced) {
   CreateAndWrapEncoder();
 
   // Configure a maximum content size sufficient for five and a half frames.
   int64 frame_bytes = kWidth * kHeight * webrtc::DesktopFrame::kBytesPerPixel;

[Peter Kasting, 2014/08/22 06:38:30]

Optional: const

[Wez, 2014/08/25 23:23:13]

Done.

   recorder_->SetMaxContentBytes((frame_bytes * 11) / 2);
 
   // Start the recorder, so that the wrapper will push frames to it.
   StartRecording();
 
   // Create frames, store them and pass them to the encoder.
   CreateTestFrames();
   EncodeTestFrames();
 
   // Only five of the supplied frames should have been recorded.
   while (test_frames_.size() > 5) {
     scoped_ptr<webrtc::DesktopFrame> frame(test_frames_.front());
     test_frames_.pop_front();
   }
 
   // Verify that the recorded frames match the ones passed to the encoder.
   VerifyTestFrames();
 }
 
 // Test that when asked to record more frames than the maximum content bytes
 // limit allows, the first encoded frames are dropped.

[Peter Kasting, 2014/08/22 06:38:31]

This comment is the same as on the previous testcase.

[Wez, 2014/08/25 23:23:13]

Done.

 TEST_F(VideoFrameRecorderTest, ContentBytesUpdatedByNextFrame) {
   CreateAndWrapEncoder();
 
   // Configure a maximum content size sufficient for kTestFrameCount frames.
   int64 frame_bytes = kWidth * kHeight * webrtc::DesktopFrame::kBytesPerPixel;
   recorder_->SetMaxContentBytes(frame_bytes * kTestFrameCount);
 
   // Start the recorder, so that the wrapper will push frames to it.
   StartRecording();
 
@@ skipping 20 matching lines @@
   EncodeTestFrames();
 
   // Clear the list of expected test frames, since none should be recorded.
   STLDeleteElements(&test_frames_);
 
   // Verify that the recorded frames match the ones passed to the encoder.
   VerifyTestFrames();
 }
 
 }  // namespace remoting
+