Allow padding in WrapVideoFrameInCVPixelBuffer

media/base/mac/video_frame_mac_unittests.cc

Index: media/base/mac/video_frame_mac_unittests.cc
diff --git a/media/base/mac/video_frame_mac_unittests.cc b/media/base/mac/video_frame_mac_unittests.cc
index c0d81091c93f0929f234c29cfb0d5f2190b4104d..aa98f0392f86f4f2898ec389eb5cc1a592f3cd7f 100644
--- a/media/base/mac/video_frame_mac_unittests.cc
+++ b/media/base/mac/video_frame_mac_unittests.cc
@@ -21,6 +21,7 @@ namespace {
 
 const int kWidth = 64;
 const int kHeight = 48;
+const int kVisibleRectOffset = 8;
 const base::TimeDelta kTimestamp = base::TimeDelta::FromMicroseconds(1337);
 
 struct FormatPair {
@@ -43,8 +44,8 @@ TEST(VideoFrameMac, CheckBasicAttributes) {
   auto pb = WrapVideoFrameInCVPixelBuffer(*frame);
   ASSERT_TRUE(pb.get());
 
-  gfx::Size coded_size = frame->coded_size();
-  VideoPixelFormat format = frame->format();
+  const gfx::Size coded_size = frame->coded_size();
+  const VideoPixelFormat format = frame->format();
 
   EXPECT_EQ(coded_size.width(), static_cast<int>(CVPixelBufferGetWidth(pb)));
   EXPECT_EQ(coded_size.height(), static_cast<int>(CVPixelBufferGetHeight(pb)));
@@ -52,7 +53,7 @@ TEST(VideoFrameMac, CheckBasicAttributes) {
 
   CVPixelBufferLockBaseAddress(pb, 0);
   for (size_t i = 0; i < VideoFrame::NumPlanes(format); ++i) {
-    gfx::Size plane_size = VideoFrame::PlaneSize(format, i, coded_size);
+    const gfx::Size plane_size = VideoFrame::PlaneSize(format, i, coded_size);
     EXPECT_EQ(plane_size.width(),
               static_cast<int>(CVPixelBufferGetWidthOfPlane(pb, i)));
     EXPECT_EQ(plane_size.height(),
@@ -115,7 +116,7 @@ TEST(VideoFrameMac, CheckWrapperFrame) {
        CoreVideoGlue::kCVPixelFormatType_420YpCbCr8BiPlanarVideoRange},
   };
 
-  gfx::Size size(kWidth, kHeight);
+  const gfx::Size size(kWidth, kHeight);
   for (const auto& format_pair : format_pairs) {
     base::ScopedCFTypeRef<CVPixelBufferRef> pb;
     CVPixelBufferCreate(nullptr, kWidth, kHeight, format_pair.corevideo,
@@ -132,4 +133,64 @@ TEST(VideoFrameMac, CheckWrapperFrame) {
   }
 }
 
+static void FillFrameWithColumnIndices(const VideoFrame& frame) {
+  for (size_t i = 0; i < VideoFrame::NumPlanes(frame.format()); ++i) {
+    const gfx::Size& size =
+        VideoFrame::PlaneSize(frame.format(), i, frame.coded_size());
+    uint8_t* plane_ptr = const_cast<uint8_t*>(frame.data(i));
+    for (int h = 0; h < size.height(); ++h) {
+      const int row_index = h * frame.stride(i);
+      for (int w = 0; w < size.width(); ++w) {
+        const uint8_t index = row_index + w;
+        plane_ptr[index] = w;

[miu, 2016/03/25 23:53:49]

We should check vertical alignment as well.  Can we make this:

  plane_ptr[index] = static_cast<uint8_t>(h ^ w);

(and change the name of this function to something like
FillFrameWithPredictableValues)

[emircan, 2016/03/29 01:33:10]

Done.

+      }
+    }
+  }
+}
+
+TEST(VideoFrameMac, CorrectlyWrapsFramesWithPadding) {
+  const gfx::Size coded_size(kWidth, kHeight);
+  const gfx::Rect visible_rect(kVisibleRectOffset, kVisibleRectOffset,
+                               kWidth - 2 * kVisibleRectOffset,
+                               kHeight - 2 * kVisibleRectOffset);
+  auto frame = VideoFrame::CreateFrame(PIXEL_FORMAT_I420, coded_size,
+                                       visible_rect, coded_size, kTimestamp);

[miu, 2016/03/25 23:53:49]

Looks like the 4th argument should be visible_rect.size().  Natural size is
almost always the visible size, unless the pixels in the video frame are not
perfect squares (i.e., no aspect ratio or scaling adjustment).

[emircan, 2016/03/29 01:33:10]

Done.

+  ASSERT_TRUE(frame.get());
+  FillFrameWithColumnIndices(*frame);
+
+  auto pb = WrapVideoFrameInCVPixelBuffer(*frame);
+  ASSERT_TRUE(pb.get());
+  EXPECT_EQ(kCVPixelFormatType_420YpCbCr8Planar,
+            CVPixelBufferGetPixelFormatType(pb));
+  EXPECT_EQ(visible_rect.width(), static_cast<int>(CVPixelBufferGetWidth(pb)));
+  EXPECT_EQ(visible_rect.height(),
+            static_cast<int>(CVPixelBufferGetHeight(pb)));
+
+  CVPixelBufferLockBaseAddress(pb, 0);
+  for (size_t i = 0; i < VideoFrame::NumPlanes(frame->format()); ++i) {
+    const gfx::Size plane_size =
+        VideoFrame::PlaneSize(frame->format(), i, visible_rect.size());
+    EXPECT_EQ(plane_size.width(),
+              static_cast<int>(CVPixelBufferGetWidthOfPlane(pb, i)));
+    EXPECT_EQ(plane_size.height(),
+              static_cast<int>(CVPixelBufferGetHeightOfPlane(pb, i)));
+
+    uint8_t* plane_ptr =
+        reinterpret_cast<uint8_t*>(CVPixelBufferGetBaseAddressOfPlane(pb, i));
+    EXPECT_EQ(frame->visible_data(i), plane_ptr);
+    const int stride =
+        static_cast<int>(CVPixelBufferGetBytesPerRowOfPlane(pb, i));
+    EXPECT_EQ(frame->stride(i), stride);
+    for (int h = 0; h < plane_size.height(); ++h) {
+      const int row_index = h * stride;
+      for (int w = 0; w < plane_size.width(); ++w) {
+        const uint8_t index = row_index + w;
+        EXPECT_EQ(frame->visible_data(i)[index],

[miu, 2016/03/25 23:53:49]

The statement on line 180 means this LOC is essentially useless (you're
comparing the values at identical locations in memory).  Instead, can we make
this:

  EXPECT_EQ(static_cast<uint8_t>((h + kVisibleRectOffset) ^ (w +
kVisibleRectOffset)), plane_ptr[index]);

[emircan, 2016/03/29 01:33:10]

Done. 

+                  static_cast<int>(plane_ptr[index]));
+      }
+    }
+  }
+  CVPixelBufferUnlockBaseAddress(pb, 0);
+}
+
 }  // namespace media