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

media/base/video_frame_unittest.cc

 // Copyright (c) 2011 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/video_frame.h"
 
 #include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stringprintf.h"
+#include "base/tuple.h"
 #include "media/base/buffers.h"
 #include "media/base/mock_filters.h"
 #include "media/base/yuv_convert.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace media {
 
 // Helper function that initializes a YV12 frame with white and black scan
 // lines based on the |white_to_black| parameter.  If 0, then the entire
 // frame will be black, if 1 then the entire frame will be white.
@@ skipping 133 matching lines @@
   uint8* u_plane = frame->data(VideoFrame::kUPlane);
   uint8* v_plane = frame->data(VideoFrame::kVPlane);
   for (size_t y = 0; y < frame->height() / 2; ++y) {
     EXPECT_EQ(0, memcmp(kExpectedUVRow, u_plane, arraysize(kExpectedUVRow)));
     EXPECT_EQ(0, memcmp(kExpectedUVRow, v_plane, arraysize(kExpectedUVRow)));
     u_plane += frame->stride(VideoFrame::kUPlane);
     v_plane += frame->stride(VideoFrame::kVPlane);
   }
 }
 
+// Ensure each frame is properly sized and allocated.  Will trigger OOB reads
+// and writes as well as incorrect frame hashes otherwise.
+TEST(VideoFrame, CheckFrameExtents) {
+  const unsigned char kFillByte = 0x80;
+  const size_t kWidth = 61;
+  const size_t kHeight = 31;
+  const base::TimeDelta kTimestampA = base::TimeDelta::FromMicroseconds(1337);
+  const base::TimeDelta kDurationA = base::TimeDelta::FromMicroseconds(1667);

[Ami GONE FROM CHROMIUM, 2012/03/22 10:53:25]

Drop the "A" from this and the previous line.

[DaleCurtis, 2012/03/22 19:45:28]

Done.

+
+  // Each entry consists of a tuple of VideoFrame::Format, # of planes, bytes
+  // per pixel, and the expected hash of all planes if filled with kFillByte.
+  const Tuple4<VideoFrame::Format, int, int, char *> kFrameTests[] = {

[Ami GONE FROM CHROMIUM, 2012/03/22 10:53:25]

s/char \*/StringPiece/
would allow you to drop the casts below (which should have been static_cast's,
if you were going to keep them, but probably the type should have been const
char * anyway to avoid the need to cast in the first place).

[DaleCurtis, 2012/03/22 19:45:28]

Dropped all this in favor of extracted function.

+    MakeTuple(
+        VideoFrame::RGB555, 1, 2, (char *)"31f7739efc76b5d9cb51361ba82533fa"),
+    MakeTuple(
+        VideoFrame::RGB565, 1, 2, (char *)"31f7739efc76b5d9cb51361ba82533fa"),
+    MakeTuple(
+        VideoFrame::RGB24,  1, 3, (char *)"84361ae9d4b6d4641a11474b3a7a2260"),
+    MakeTuple(
+        VideoFrame::RGB32,  1, 4, (char *)"de6d3d567e282f6a38d478f04fc81fb0"),
+    MakeTuple(
+        VideoFrame::RGBA,   1, 4, (char *)"de6d3d567e282f6a38d478f04fc81fb0"),
+    MakeTuple(
+        VideoFrame::YV12,   3, 1, (char *)"71113bdfd4c0de6cf62f48fb74f7a0b1"),
+    MakeTuple(
+        VideoFrame::YV16,   3, 1, (char *)"9bb99ac3ff350644ebff4d28dc01b461"),
+  };
+
+  base::MD5Digest digest;

[Ami GONE FROM CHROMIUM, 2012/03/22 10:53:25]

This and the next line can go near the bottom of the loop (where they are first
used)

[DaleCurtis, 2012/03/22 19:45:28]

Done.

+  base::MD5Context context;
+  for(unsigned long i = 0; i < arraysize(kFrameTests); i++) {

[Ami GONE FROM CHROMIUM, 2012/03/22 10:53:25]

If it were me I'd extract the loop body into a helper function, because that
would replace the need for the kFrameTests array above (with its MakeTuple()s)
with simpler function calls
TestSizes(VideoFrame::RGB32, 1, 4, "de6d3d567e282f6a38d478f04fc81fb0");
etc.

[DaleCurtis, 2012/03/22 19:45:28]

Done.

+    scoped_refptr<VideoFrame> frame = VideoFrame::CreateFrame(

[Ami GONE FROM CHROMIUM, 2012/03/22 10:53:25]

Add 
SCOPED_TRACE(IntToString(i));
for easier debugging.

[DaleCurtis, 2012/03/22 19:45:28]

Done.

+        kFrameTests[i].a, kWidth, kHeight, kTimestampA, kDurationA);
+    ASSERT_TRUE(frame);
+
+    // 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.
+    for(int plane = 0; plane < kFrameTests[i].b; 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 * kFrameTests[i].c);
+      }
+
+      memset(frame->data(plane), kFillByte,
+             frame->stride(plane) * frame->rows(plane));
+    }
+
+    base::MD5Init(&context);
+    frame->HashFrameForTesting(&context);
+    base::MD5Final(&digest, &context);
+    EXPECT_EQ(MD5DigestToBase16(digest), kFrameTests[i].d);
+  }
+}
+
 }  // namespace media