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/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stringprintf.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
@@ skipping 98 matching lines @@
     SCOPED_TRACE("");
     InitializeYV12Frame(frame, 1.0f);
     ExpectFrameColor(frame, 0xFFFFFFFF);
   }
 
   // Test an empty frame.
   frame = VideoFrame::CreateEmptyFrame();
   EXPECT_TRUE(frame->IsEndOfStream());
 }
 
+static inline size_t RoundUp(size_t value, size_t alignment) {

[Ami GONE FROM CHROMIUM, 2012/03/21 04:33:44]

If inline was extraneous this could easily become a public (*ForTest()) static
helper method of VideoFrame and avoid the duplication.
Wanna check perf impact of this?  I suspect it's nil.

[DaleCurtis, 2012/03/22 01:24:43]

Aside from the DCHECK it's just a simple return method, is it bad form to static
inline the whole thing in the header file? The new test doesn't use this, so
I've left it alone.

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

Yes.
Cool.

+  // Check that |alignment| is a power of 2.
+  DCHECK((alignment + (alignment - 1)) == (alignment | (alignment - 1)));

[Ami GONE FROM CHROMIUM, 2012/03/21 04:33:44]

s/DCHECK/ASSERT_TRUE/

(this is something I'm guilty of too; chromium's test infra makes it much
friendlier to assert than to dcheck)

+  return ((value + (alignment - 1)) & ~(alignment-1));
+}
+
 TEST(VideoFrame, CreateBlackFrame) {
   const size_t kWidth = 2;
   const size_t kHeight = 2;
   const uint8 kExpectedYRow[] = { 0, 0 };
   const uint8 kExpectedUVRow[] = { 128 };
 
   scoped_refptr<media::VideoFrame> frame =
       VideoFrame::CreateBlackFrame(kWidth, kHeight);
   ASSERT_TRUE(frame);
 
@@ skipping 17 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);
   }
 }
 
+TEST(VideoFrame, CheckRowSizes) {

[DaleCurtis, 2012/03/20 22:54:06]

A bit lengthy, but the generic version proved just as long and harder to read:

http://pastebin.com/tTYQJqXS

+  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);
+
+  // Ensure each frame format yields the correct number of rows and row_bytes.

[Ami GONE FROM CHROMIUM, 2012/03/21 04:33:44]

This comment belongs at the top of the TEST().
But it's actually misleading IMO; most (all?) of the RoundUp business is
implementation artifact for performance reasons, not an artifact of
"correctness" of the frame.  If we shipped on a 128-bit platform we might bump
some of the rounding, but it'd be hard to justify changing the test to match
with anything other than "this is what the code actually emits".

IMO this test is the wrong way to go about it.
Instead, to assert the values are correct, *use* the values for whatever they'd
be used for by a client, and assert that the result is correct.
For example, if you copied the data out of the planes into another buffer using
the accessors in question to bound the loops, and then checked the resulting
buffers for having the expected bits at least in the visible bytes, you'd be
able to:
- ensure the accessors return values large enough to account for the visible
bytes
- ensure the accessors don't return wildly-too-large values (b/c memcheck/asan
bots would 'splode)

WDYT?

[DaleCurtis, 2012/03/22 01:24:43]

I think it sounds great and was similar to what I was thinking, but didn't do.
I've written up a new test case which is a mix of the old and new. Let me know
what you think.

+  scoped_refptr<VideoFrame> frame = VideoFrame::CreateFrame(
+      VideoFrame::RGB555, kWidth, kHeight, kTimestampA, kDurationA);
+  EXPECT_EQ(frame->rows(VideoFrame::kRGBPlane), kHeight);
+  EXPECT_EQ(frame->row_bytes(VideoFrame::kRGBPlane), kWidth * 2);
+  EXPECT_EQ(frame->stride(VideoFrame::kRGBPlane), RoundUp(kWidth * 2, 8));
+
+  frame = VideoFrame::CreateFrame(
+      VideoFrame::RGB565, kWidth, kHeight, kTimestampA, kDurationA);
+  EXPECT_EQ(frame->rows(VideoFrame::kRGBPlane), kHeight);
+  EXPECT_EQ(frame->row_bytes(VideoFrame::kRGBPlane), kWidth * 2);
+  EXPECT_EQ(frame->stride(VideoFrame::kRGBPlane), RoundUp(kWidth * 2, 8));
+
+  frame = VideoFrame::CreateFrame(
+      VideoFrame::RGB24, kWidth, kHeight, kTimestampA, kDurationA);
+  EXPECT_EQ(frame->rows(VideoFrame::kRGBPlane), kHeight);
+  EXPECT_EQ(frame->row_bytes(VideoFrame::kRGBPlane), kWidth * 3);
+  EXPECT_EQ(frame->stride(VideoFrame::kRGBPlane), RoundUp(kWidth * 3, 8));
+
+  frame = VideoFrame::CreateFrame(
+      VideoFrame::RGB32, kWidth, kHeight, kTimestampA, kDurationA);
+  EXPECT_EQ(frame->rows(VideoFrame::kRGBPlane), kHeight);
+  EXPECT_EQ(frame->row_bytes(VideoFrame::kRGBPlane), kWidth * 4);
+  EXPECT_EQ(frame->stride(VideoFrame::kRGBPlane), RoundUp(kWidth * 4, 8));
+
+  frame = VideoFrame::CreateFrame(
+      VideoFrame::RGBA, kWidth, kHeight, kTimestampA, kDurationA);
+  EXPECT_EQ(frame->rows(VideoFrame::kRGBPlane), kHeight);
+  EXPECT_EQ(frame->row_bytes(VideoFrame::kRGBPlane), kWidth * 4);
+  EXPECT_EQ(frame->stride(VideoFrame::kRGBPlane), RoundUp(kWidth * 4, 8));
+
+  frame = VideoFrame::CreateFrame(
+      VideoFrame::YV12, kWidth, kHeight, kTimestampA, kDurationA);
+  EXPECT_EQ(frame->rows(VideoFrame::kYPlane), kHeight);
+  EXPECT_EQ(frame->rows(VideoFrame::kUPlane), RoundUp(kHeight, 2) / 2);
+  EXPECT_EQ(frame->rows(VideoFrame::kVPlane), RoundUp(kHeight, 2) / 2);
+  EXPECT_EQ(frame->row_bytes(VideoFrame::kYPlane), kWidth);
+  EXPECT_EQ(frame->row_bytes(VideoFrame::kUPlane), RoundUp(kWidth, 2) / 2);
+  EXPECT_EQ(frame->row_bytes(VideoFrame::kVPlane), RoundUp(kWidth, 2) / 2);
+  EXPECT_EQ(frame->stride(VideoFrame::kYPlane), RoundUp(frame->row_bytes(
+      VideoFrame::kYPlane), 4));
+  EXPECT_EQ(frame->stride(VideoFrame::kUPlane), RoundUp(frame->row_bytes(
+      VideoFrame::kUPlane), 4));
+  EXPECT_EQ(frame->stride(VideoFrame::kVPlane), RoundUp(frame->row_bytes(
+      VideoFrame::kVPlane), 4));
+
+  frame = VideoFrame::CreateFrame(
+      VideoFrame::YV16, kWidth, kHeight, kTimestampA, kDurationA);
+  EXPECT_EQ(frame->rows(VideoFrame::kYPlane), kHeight);
+  EXPECT_EQ(frame->rows(VideoFrame::kUPlane), kHeight);
+  EXPECT_EQ(frame->rows(VideoFrame::kVPlane), kHeight);
+  EXPECT_EQ(frame->row_bytes(VideoFrame::kYPlane), kWidth);
+  EXPECT_EQ(frame->row_bytes(VideoFrame::kUPlane), RoundUp(kWidth, 2) / 2);
+  EXPECT_EQ(frame->row_bytes(VideoFrame::kVPlane), RoundUp(kWidth, 2) / 2);
+  EXPECT_EQ(frame->stride(VideoFrame::kYPlane), RoundUp(frame->row_bytes(
+      VideoFrame::kYPlane), 4));
+  EXPECT_EQ(frame->stride(VideoFrame::kUPlane), RoundUp(frame->row_bytes(
+      VideoFrame::kUPlane), 4));
+  EXPECT_EQ(frame->stride(VideoFrame::kVPlane), RoundUp(frame->row_bytes(
+      VideoFrame::kVPlane), 4));
+}
+
 }  // namespace media