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..975f016eeda829196fd5ea81c655e0a4a4a91cc3 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 = 4;
 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,34 @@ TEST(VideoFrameMac, CheckWrapperFrame) {
   }
 }
 
+TEST(VideoFrameMac, CheckFrameWithVisibleRect) {

[miu, 2016/03/24 20:29:00]

nit: Please rename to something more meaningful.  Suggestion:
CorrectlyWrapsFramesWithPadding

[emircan, 2016/03/25 07:43:22]

Done.

+  const gfx::Size size(kWidth, kHeight);

[miu, 2016/03/24 20:29:00]

nit: Please name this coded_size for clarity.

[emircan, 2016/03/25 07:43:22]

Done.

+  const gfx::Rect visible_rect(kVisibleRectOffset, kVisibleRectOffset,
+                               kWidth - 2 * kVisibleRectOffset,
+                               kHeight - 2 * kVisibleRectOffset);
+  auto frame = VideoFrame::CreateFrame(PIXEL_FORMAT_I420, size, visible_rect,
+                                       size, kTimestamp);
+  ASSERT_TRUE(frame.get());
+  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)));
+    EXPECT_EQ(frame->visible_data(i),
+              CVPixelBufferGetBaseAddressOfPlane(pb, i));
+  }

[miu, 2016/03/24 20:29:00]

Also, you should check the value returned by
CVPixelBufferGetBytesPerRowOfPlane() matches the coded size (for the Y plane;
1/2 size for U and V).

[miu, 2016/03/24 20:29:00]

Per earlier discussion, I think it would be good to read the bytes in the plane,
checking for an expected pattern.

[emircan, 2016/03/25 07:43:22]

Ok, that should equals stride as well.

[emircan, 2016/03/25 07:43:22]

Done.

+  CVPixelBufferUnlockBaseAddress(pb, 0);
+}
+
 }  // namespace media