Fix incorrect VideoFrame::row_bytes() for RGB. Add tests.

Also converts the Copy*Plane methods in video_util.cc to use size_t
instead of int.

BUG=119156
TEST=Built chrome. Ran unittests.


Review URL: http://codereview.chromium.org/9766007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128379 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 70 insertions, 1 deletions

diff --git a/media/base/video_frame.cc b/media/base/video_frame.cc
index 750c2770..441a49c 100644
--- a/media/base/video_frame.cc
+++ b/media/base/video_frame.cc
@@ -207,13 +207,21 @@ int VideoFrame::stride(size_t plane) const {
 int VideoFrame::row_bytes(size_t plane) const {
   DCHECK(IsValidPlane(plane));
   switch (format_) {
+    // 16bpp.
     case RGB555:
     case RGB565:
+      return width_ * 2;
+
+    // 24bpp.
     case RGB24:
+      return width_ * 3;
+
+    // 32bpp.
     case RGB32:
     case RGBA:
-      return width_;
+      return width_ * 4;
 
+    // Planar, 8bpp.
     case YV12:
     case YV16:
       if (plane == kYPlane)
diff --git a/media/base/video_frame_unittest.cc b/media/base/video_frame_unittest.cc
index 5a6c0cd..83d4c73 100644
--- a/media/base/video_frame_unittest.cc
+++ b/media/base/video_frame_unittest.cc
@@ -79,6 +79,45 @@ void ExpectFrameColor(media::VideoFrame* yv12_frame, uint32 expect_rgb_color) {
   }
 }
 
+// Fill each plane to its reported extents and verify accessors report non
+// zero values.  Additionally, for the first plane verify the rows and
+// row_bytes values are correct.
+void ExpectFrameExtents(VideoFrame::Format format, int planes,
+                        int bytes_per_pixel, const char* expected_hash) {
+  const unsigned char kFillByte = 0x80;
+  const size_t kWidth = 61;
+  const size_t kHeight = 31;
+  const base::TimeDelta kTimestamp = base::TimeDelta::FromMicroseconds(1337);
+  const base::TimeDelta kDuration = base::TimeDelta::FromMicroseconds(1667);
+
+  scoped_refptr<VideoFrame> frame = VideoFrame::CreateFrame(
+      format, kWidth, kHeight, kTimestamp, kDuration);
+  ASSERT_TRUE(frame);
+
+  for(int plane = 0; plane < planes; plane++) {
+    SCOPED_TRACE(base::StringPrintf("Checking plane %d", plane));
+    EXPECT_TRUE(frame->data(plane));
+    EXPECT_TRUE(frame->stride(plane));
+    EXPECT_TRUE(frame->rows(plane));
+    EXPECT_TRUE(frame->row_bytes(plane));
+
+    if (plane == 0) {
+      EXPECT_EQ((size_t)frame->rows(plane), kHeight);
+      EXPECT_EQ((size_t)frame->row_bytes(plane), kWidth * bytes_per_pixel);
+    }
+
+    memset(frame->data(plane), kFillByte,
+           frame->stride(plane) * frame->rows(plane));
+  }
+
+  base::MD5Context context;
+  base::MD5Init(&context);
+  frame->HashFrameForTesting(&context);
+  base::MD5Digest digest;
+  base::MD5Final(&digest, &context);
+  EXPECT_EQ(MD5DigestToBase16(digest), expected_hash);
+}
+
 TEST(VideoFrame, CreateFrame) {
   const size_t kWidth = 64;
   const size_t kHeight = 48;
@@ -172,4 +211,26 @@ TEST(VideoFrame, CreateBlackFrame) {
   }
 }
 
+// Ensure each frame is properly sized and allocated.  Will trigger OOB reads
+// and writes as well as incorrect frame hashes otherwise.
+TEST(VideoFrame, CheckFrameExtents) {
+  // Each call consists of a VideoFrame::Format, # of planes, bytes per pixel,
+  // and the expected hash of all planes if filled with kFillByte (defined in
+  // ExpectFrameExtents).
+  ExpectFrameExtents(
+      VideoFrame::RGB555, 1, 2, "31f7739efc76b5d9cb51361ba82533fa");
+  ExpectFrameExtents(
+      VideoFrame::RGB565, 1, 2, "31f7739efc76b5d9cb51361ba82533fa");
+  ExpectFrameExtents(
+      VideoFrame::RGB24,  1, 3, "84361ae9d4b6d4641a11474b3a7a2260");
+  ExpectFrameExtents(
+      VideoFrame::RGB32,  1, 4, "de6d3d567e282f6a38d478f04fc81fb0");
+  ExpectFrameExtents(
+      VideoFrame::RGBA,   1, 4, "de6d3d567e282f6a38d478f04fc81fb0");
+  ExpectFrameExtents(
+      VideoFrame::YV12,   3, 1, "71113bdfd4c0de6cf62f48fb74f7a0b1");
+  ExpectFrameExtents(
+      VideoFrame::YV16,   3, 1, "9bb99ac3ff350644ebff4d28dc01b461");
+}
+
 }  // namespace media