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..1f3d7fd6074bab5f53687f091ef1b3120669390d 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,61 @@ TEST(VideoFrameMac, CheckWrapperFrame) {
   }
 }
 
+static void FillFrameWithColumnIndices(const scoped_refptr<VideoFrame>& frame) {

[miu, 2016/03/25 21:38:37]

nit: Please pass the argument as const VideoFrame&.

[emircan, 2016/03/25 23:32:09]

Done. Note that I need to const_cast the data() ptrs in that case.

+  for (size_t i = 0; i < VideoFrame::NumPlanes(frame->format()); ++i) {
+    const gfx::Size& size =
+        VideoFrame::PlaneSize(frame->format(), i, frame->coded_size());

[miu, 2016/03/25 21:38:37]

You meant frame->visible_rect().size() as the 3rd arg to PlaneSize(), right?

[emircan, 2016/03/25 23:32:09]

Actually, I wanted to fill intentionally the whole coded_size. Visible rect
starts from kVisibleRectOffset and if there is an shift, it is very much visible
from values.

+    for (int h = 0; h < size.height(); ++h) {
+      for (int w = 0; w < size.width(); ++w) {
+        const uint8_t index = h * frame->stride(i) + w;

[miu, 2016/03/25 21:38:37]

nit: The |h * frame->stride(i)| part could be moved above, before this inner
loop to avoid redundant computations.  Being pedantic about the performance of
unit tests is important since they hold up the bots, CQ, etc.  ;-)

[emircan, 2016/03/25 23:32:09]

Done.

+        frame->data(i)[index] = w;

[miu, 2016/03/25 21:38:37]

You meant to call visible_data() here, right?

[emircan, 2016/03/25 23:32:09]

I would rather fill up the whole coded_size() as I explained above.

+      }
+    }
+  }
+}
+
+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);
+  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) {
+      for (int w = 0; w < plane_size.width(); ++w) {
+        const uint8_t index = h * stride + w;

[miu, 2016/03/25 21:38:37]

ditto: Please move |h * stride| part of the calculation to the outer loop.

[emircan, 2016/03/25 23:32:09]

Done.

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