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

media/base/video_frame.cc

Index: media/base/video_frame.cc
diff --git a/media/base/video_frame.cc b/media/base/video_frame.cc
index fc977364417280706f5dadd95ad58d47b299408e..9d154d04c990a4eced29c065ac7e57a0d56f2adf 100644
--- a/media/base/video_frame.cc
+++ b/media/base/video_frame.cc
@@ -198,21 +198,29 @@ bool VideoFrame::IsValidPlane(size_t plane) const {
   return false;
 }
 
-int VideoFrame::stride(size_t plane) const {
+size_t VideoFrame::stride(size_t plane) const {
   DCHECK(IsValidPlane(plane));
   return strides_[plane];
 }
 
-int VideoFrame::row_bytes(size_t plane) const {
+size_t VideoFrame::row_bytes(size_t plane) const {
   DCHECK(IsValidPlane(plane));
   switch (format_) {
+    // 16bpp.
     case RGB555:
     case RGB565:
+      return width_ * 2;

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

FTR, nothing uses these, right?
(i.e. this a rake-in-the-grass waiting for someone to use it, but there's no
triggerable bug here now)

You might be interested in
http://code.google.com/p/chromium/issues/detail?id=108306 ;)

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

None that I could find. I like the rake in the grass analogy :)

After seeing that bug I did a little more digging and I'm pretty sure all of the
formats are being used at least superficially in one way or another in tests,
remoting, decoding, etc.  I'm not sure we can pare this down without creating
separate enums and refactoring everyone.

Why are we not just using WebKit::WebVideoFrame directly here?

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

Oh, sad panda.
media/ shouldn't depend on third_party/WebKit.

[scherkus (not reviewing), 2012/03/22 10:57:54]

I believe all usage of these additional formats are purely superficial.

Are you sure refactoring is necessary as opposed to deleting all
references/tests or is that what you're referring to when you say refactoring?
That would be a layering violation.

[DaleCurtis, 2012/03/22 17:30:36]

To be precise, I took my notes from the previous search on
"VideoFrame::<format>" and "<format>" and did some additional tracing:

Used in production, testing or tools:
INVALID
RGB32
RGBA
YV12
YV16
EMPTY
I420
NATIVE_TEXTURE

Used in testing:
ASCII

Used superficially, but unnecessary:
RGB565
RGB24
NV12

Unused (mistook for another enum on the first search):
RGB555

When I said refactoring I meant if we introduced a new enum or wanted to replace
usage of a format. Culling RGB565, RGB24, NV12, and RGB555 can be done easily,
we'll just have to manually assign values in the enum for the gaps they leave.
If that's desirable, I'll take the bug mentioned above and create another CL.

+
+    // 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)
@@ -228,7 +236,7 @@ int VideoFrame::row_bytes(size_t plane) const {
   return 0;
 }
 
-int VideoFrame::rows(size_t plane) const {
+size_t VideoFrame::rows(size_t plane) const {
   DCHECK(IsValidPlane(plane));
   switch (format_) {
     case RGB555: