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

media/base/video_frame.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/video_frame.h"
 
 #include "base/logging.h"
 #include "media/base/limits.h"
 #include "media/base/video_util.h"
 
@@ skipping 180 matching lines @@
 
     default:
       break;
   }
 
   // Intentionally leave out non-production formats.
   NOTREACHED() << "Unsupported video frame format: " << format_;
   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)
         return width_;
       return RoundUp(width_, 2) / 2;
 
     default:
       break;
   }
 
   // Intentionally leave out non-production formats.
   NOTREACHED() << "Unsupported video frame format: " << format_;
   return 0;
 }
 
-int VideoFrame::rows(size_t plane) const {
+size_t VideoFrame::rows(size_t plane) const {
   DCHECK(IsValidPlane(plane));
   switch (format_) {
     case RGB555:
     case RGB565:
     case RGB24:
     case RGB32:
     case RGBA:
     case YV16:
       return height_;
 
@@ skipping 24 matching lines @@
 uint32 VideoFrame::texture_target() const {
   DCHECK_EQ(format_, NATIVE_TEXTURE);
   return texture_target_;
 }
 
 bool VideoFrame::IsEndOfStream() const {
   return format_ == VideoFrame::EMPTY;
 }
 
 }  // namespace media