Allow padding in WrapVideoFrameInCVPixelBuffer

media/base/mac/video_frame_mac_unittests.cc

 // Copyright (c) 2012 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 "media/base/mac/video_frame_mac.h"
 
 #include <stddef.h>
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "media/base/mac/corevideo_glue.h"
 #include "media/base/video_frame.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace media {
 
 namespace {
 
 const int kWidth = 64;
 const int kHeight = 48;
+const int kVisibleRectOffset = 8;
 const base::TimeDelta kTimestamp = base::TimeDelta::FromMicroseconds(1337);
 
 struct FormatPair {
   VideoPixelFormat chrome;
   OSType corevideo;
 };
 
 void Increment(int* i) {
   ++(*i);
 }
 
 }  // namespace
 
 TEST(VideoFrameMac, CheckBasicAttributes) {
   gfx::Size size(kWidth, kHeight);
   auto frame = VideoFrame::CreateFrame(PIXEL_FORMAT_I420, size, gfx::Rect(size),
                                        size, kTimestamp);
   ASSERT_TRUE(frame.get());
 
   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)));
   EXPECT_EQ(VideoFrame::NumPlanes(format), CVPixelBufferGetPlaneCount(pb));
 
   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(),
               static_cast<int>(CVPixelBufferGetHeightOfPlane(pb, i)));
     EXPECT_EQ(frame->data(i), CVPixelBufferGetBaseAddressOfPlane(pb, i));
   }
   CVPixelBufferUnlockBaseAddress(pb, 0);
 }
 
 TEST(VideoFrameMac, CheckFormats) {
@@ skipping 42 matching lines @@
   EXPECT_EQ(1, instances_destroyed);
 }
 
 TEST(VideoFrameMac, CheckWrapperFrame) {
   const FormatPair format_pairs[] = {
       {PIXEL_FORMAT_I420, kCVPixelFormatType_420YpCbCr8Planar},
       {PIXEL_FORMAT_NV12,
        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,
                         nullptr, pb.InitializeInto());
     ASSERT_TRUE(pb.get());
 
     auto frame = VideoFrame::WrapCVPixelBuffer(pb.get(), kTimestamp);
     ASSERT_TRUE(frame.get());
     EXPECT_EQ(pb.get(), frame->cv_pixel_buffer());
     EXPECT_EQ(format_pair.chrome, frame->format());
 
     frame = nullptr;
     EXPECT_EQ(1, CFGetRetainCount(pb.get()));
   }
 }
 
+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