Audit/fix use of media::VideoFrame::coded_size()

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 7e654319c52c9964250023eb8ea2ea83bff85802..785d5dc8cfdbd906bfb09b09261798fdbbe3f95f 100644
--- a/media/base/video_frame.cc
+++ b/media/base/video_frame.cc
@@ -18,6 +18,12 @@
 
 namespace media {
 
+static inline size_t RoundUp(size_t value, size_t alignment) {
+  // Check that |alignment| is a power of 2.
+  DCHECK((alignment + (alignment - 1)) == (alignment | (alignment - 1)));
+  return ((value + (alignment - 1)) & ~(alignment - 1));
+}
+
 // static
 scoped_refptr<VideoFrame> VideoFrame::CreateFrame(
     VideoFrame::Format format,
@@ -25,20 +31,28 @@ scoped_refptr<VideoFrame> VideoFrame::CreateFrame(
     const gfx::Rect& visible_rect,
     const gfx::Size& natural_size,
     base::TimeDelta timestamp) {
-  DCHECK(IsValidConfig(format, coded_size, visible_rect, natural_size));
-  scoped_refptr<VideoFrame> frame(new VideoFrame(
-      format, coded_size, visible_rect, natural_size, timestamp, false));
+  // Since we're creating a new YUV frame (and allocating memory for it
+  // ourselves), we can pad the requested |coded_size| if necessary if the
+  // request does not line up on sample boundaries.
+  gfx::Size new_coded_size(coded_size);
   switch (format) {
     case VideoFrame::YV12:
     case VideoFrame::YV12A:
-    case VideoFrame::YV16:
     case VideoFrame::I420:
     case VideoFrame::YV12J:
-      frame->AllocateYUV();
+      new_coded_size.set_height((new_coded_size.height() + 1) / 2 * 2);
+    // Fallthrough.
+    case VideoFrame::YV16:
+      new_coded_size.set_width((new_coded_size.width() + 1) / 2 * 2);
       break;
     default:
-      LOG(FATAL) << "Unsupported frame format: " << format;
+      LOG(FATAL) << "Only YUV formats supported: " << format;
+      return NULL;
   }
+  DCHECK(IsValidConfig(format, new_coded_size, visible_rect, natural_size));
+  scoped_refptr<VideoFrame> frame(new VideoFrame(
+      format, new_coded_size, visible_rect, natural_size, timestamp, false));
+  frame->AllocateYUV();
   return frame;
 }
 
@@ -75,19 +89,46 @@ bool VideoFrame::IsValidConfig(VideoFrame::Format format,
                                const gfx::Size& coded_size,
                                const gfx::Rect& visible_rect,
                                const gfx::Size& natural_size) {
-  return (format != VideoFrame::UNKNOWN &&
-          !coded_size.IsEmpty() &&
-          coded_size.GetArea() <= limits::kMaxCanvas &&
-          coded_size.width() <= limits::kMaxDimension &&
-          coded_size.height() <= limits::kMaxDimension &&
-          !visible_rect.IsEmpty() &&
-          visible_rect.x() >= 0 && visible_rect.y() >= 0 &&
-          visible_rect.right() <= coded_size.width() &&
-          visible_rect.bottom() <= coded_size.height() &&
-          !natural_size.IsEmpty() &&
-          natural_size.GetArea() <= limits::kMaxCanvas &&
-          natural_size.width() <= limits::kMaxDimension &&
-          natural_size.height() <= limits::kMaxDimension);
+  switch (format) {
+    case VideoFrame::UNKNOWN:
+      return (coded_size.IsEmpty() && visible_rect.IsEmpty() &&
+              natural_size.IsEmpty());
+    case VideoFrame::YV12:
+    case VideoFrame::YV12J:
+    case VideoFrame::I420:
+    case VideoFrame::YV12A:
+      // YUV formats have width/height requirements due to chroma subsampling.
+      if (static_cast<size_t>(coded_size.height()) <
+          RoundUp(visible_rect.bottom(), 2))

[Ami GONE FROM CHROMIUM, 2014/02/28 23:17:32]

Subtle!
So it's ok for coded size to be odd as long as visible_rect is away from the
oddness?  The relevant codecs specify how to deal with this?

[sheu, 2014/03/01 01:56:57]

The relevant codecs already allow for odd-sized coded_size (e.g. ffmpeg may
choose to output an odd width/height _and_ coded_width/coded_height).  So yes,
if we keep visible_rect away from it we should be safe.

+        return false;
+    // Fallthrough.
+    case VideoFrame::YV16:
+      if (static_cast<size_t>(coded_size.width()) <
+          RoundUp(visible_rect.right(), 2))
+        return false;
+      break;
+    case VideoFrame::NATIVE_TEXTURE:
+#if defined(VIDEO_HOLE)
+    case VideoFrame::HOLE:
+#endif  // defined(VIDEO_HOLE)
+      break;
+    case VideoFrame::HISTOGRAM_MAX:
+      NOTREACHED();
+      return false;
+  }
+
+  if (coded_size.IsEmpty() || coded_size.GetArea() > limits::kMaxCanvas ||
+      coded_size.width() > limits::kMaxDimension ||
+      coded_size.height() > limits::kMaxDimension || visible_rect.IsEmpty() ||
+      visible_rect.x() < 0 || visible_rect.y() < 0 ||
+      visible_rect.right() > coded_size.width() ||
+      visible_rect.bottom() > coded_size.height() || natural_size.IsEmpty() ||
+      natural_size.GetArea() > limits::kMaxCanvas ||
+      natural_size.width() > limits::kMaxDimension ||
+      natural_size.height() > limits::kMaxDimension)
+    return false;
+
+  return true;
 }
 
 // static
@@ -129,6 +170,8 @@ scoped_refptr<VideoFrame> VideoFrame::WrapExternalPackedMemory(
     base::SharedMemoryHandle handle,
     base::TimeDelta timestamp,
     const base::Closure& no_longer_needed_cb) {
+  if (!IsValidConfig(format, coded_size, visible_rect, natural_size))
+    return NULL;
   if (data_size < AllocationSize(format, coded_size))
     return NULL;
 
@@ -166,7 +209,9 @@ scoped_refptr<VideoFrame> VideoFrame::WrapExternalYuvData(
     uint8* v_data,
     base::TimeDelta timestamp,
     const base::Closure& no_longer_needed_cb) {
-  DCHECK(format == YV12 || format == YV16 || format == I420) << format;
+  if (!IsValidConfig(format, coded_size, visible_rect, natural_size))
+    return NULL;
+
   scoped_refptr<VideoFrame> frame(new VideoFrame(
       format, coded_size, visible_rect, natural_size, timestamp, false));
   frame->strides_[kYPlane] = y_stride;
@@ -211,7 +256,6 @@ scoped_refptr<VideoFrame> VideoFrame::CreateColorFrame(
     const gfx::Size& size,
     uint8 y, uint8 u, uint8 v,
     base::TimeDelta timestamp) {
-  DCHECK(IsValidConfig(VideoFrame::YV12, size, gfx::Rect(size), size));
   scoped_refptr<VideoFrame> frame = VideoFrame::CreateFrame(
       VideoFrame::YV12, size, gfx::Rect(size), size, timestamp);
   FillYUV(frame.get(), y, u, v);
@@ -267,11 +311,6 @@ size_t VideoFrame::NumPlanes(Format format) {
   return 0;
 }
 
-static inline size_t RoundUp(size_t value, size_t alignment) {
-  // Check that |alignment| is a power of 2.
-  DCHECK((alignment + (alignment - 1)) == (alignment | (alignment - 1)));
-  return ((value + (alignment - 1)) & ~(alignment-1));
-}
 
 // static
 size_t VideoFrame::AllocationSize(Format format, const gfx::Size& coded_size) {
@@ -282,21 +321,21 @@ size_t VideoFrame::AllocationSize(Format format, const gfx::Size& coded_size) {
 }
 
 // static
-size_t VideoFrame::PlaneAllocationSize(Format format,
-                                       size_t plane,
-                                       const gfx::Size& coded_size) {
-  const size_t area =
-      RoundUp(coded_size.width(), 2) * RoundUp(coded_size.height(), 2);
+gfx::Size VideoFrame::PlaneSize(Format format,
+                                size_t plane,
+                                const gfx::Size& coded_size) {
+  const int width = RoundUp(coded_size.width(), 2);
+  const int height = RoundUp(coded_size.height(), 2);
   switch (format) {
     case VideoFrame::YV12:
     case VideoFrame::YV12J:
     case VideoFrame::I420: {
       switch (plane) {
         case VideoFrame::kYPlane:
-          return area;
+          return gfx::Size(width, height);
         case VideoFrame::kUPlane:
         case VideoFrame::kVPlane:
-          return area / 4;
+          return gfx::Size(width / 2, height / 2);
         default:
           break;
       }
@@ -305,10 +344,10 @@ size_t VideoFrame::PlaneAllocationSize(Format format,
       switch (plane) {
         case VideoFrame::kYPlane:
         case VideoFrame::kAPlane:
-          return area;
+          return gfx::Size(width, height);
         case VideoFrame::kUPlane:
         case VideoFrame::kVPlane:
-          return area / 4;
+          return gfx::Size(width / 2, height / 2);
         default:
           break;
       }
@@ -316,10 +355,10 @@ size_t VideoFrame::PlaneAllocationSize(Format format,
     case VideoFrame::YV16: {
       switch (plane) {
         case VideoFrame::kYPlane:
-          return area;
+          return gfx::Size(width, height);
         case VideoFrame::kUPlane:
         case VideoFrame::kVPlane:
-          return area / 2;
+          return gfx::Size(width / 2, height);
         default:
           break;
       }
@@ -334,7 +373,14 @@ size_t VideoFrame::PlaneAllocationSize(Format format,
   }
   NOTREACHED() << "Unsupported video frame format/plane: "
                << format << "/" << plane;
-  return 0;
+  return gfx::Size();
+}
+
+size_t VideoFrame::PlaneAllocationSize(Format format,
+                                       size_t plane,
+                                       const gfx::Size& coded_size) {
+  // VideoFrame formats are (so far) all YUV and 1 byte per sample.
+  return PlaneSize(format, plane, coded_size).GetArea();
 }
 
 // Release data allocated by AllocateYUV().
@@ -410,6 +456,8 @@ VideoFrame::VideoFrame(VideoFrame::Format format,
       shared_memory_handle_(base::SharedMemory::NULLHandle()),
       timestamp_(timestamp),
       end_of_stream_(end_of_stream) {
+  DCHECK(IsValidConfig(format_, coded_size_, visible_rect_, natural_size_));
+
   memset(&strides_, 0, sizeof(strides_));
   memset(&data_, 0, sizeof(data_));
 }