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..1e13b4d171f0455f87c005e44991aab5413902de 100644
--- a/media/base/video_frame.cc
+++ b/media/base/video_frame.cc
@@ -25,20 +25,27 @@ 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.

[Ami GONE FROM CHROMIUM, 2014/02/27 01:20:12]

"necessary" for what?

[sheu, 2014/02/28 00:40:19]

Added commentary.

+  gfx::Size new_coded_size(coded_size);

[Ami GONE FROM CHROMIUM, 2014/02/27 01:20:12]

nit: when making a change like this I prefer to rename the function parameter
(e.g. orig_coded_size) and name the new thing the old name (e.g. this local var
would be coded_size) to ensure no old uses remain.

[sheu, 2014/02/28 00:40:19]

I like to make the header declaration and the function definition match up.  As
before, let me know if this really needs to change.

[Ami GONE FROM CHROMIUM, 2014/02/28 00:48:13]

Meh.
(your way is better for cleaner signatures, my way is better for less chance of
future bugs)

   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;

[Ami GONE FROM CHROMIUM, 2014/02/27 01:20:12]

Is there not enough __noreturn__ whateverness on LOG(FATAL) that this can be
omitted?

[sheu, 2014/02/28 00:40:19]

Nope.  Still has to return something.

   }
+  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 +82,40 @@ 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);
+  if (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)
+    return false;
+
+  // YUV formats have width/height requirements due to chroma subsampling.
+  switch (format) {
+    case VideoFrame::YV12:
+    case VideoFrame::YV12J:
+    case VideoFrame::I420:
+    case VideoFrame::YV12A:
+      if (coded_size.height() % 2 != 0)

[Ami GONE FROM CHROMIUM, 2014/02/27 01:20:12]

I almost want to CHECK this...

[sheu, 2014/02/28 00:40:19]

Well, for the cases where we only DCHECK(IsValidConfig()), we'd get the same
behavior.  For the cases where we're concerned enough to test it directly
(WrapExternalPackedMemory() and WrapExternalYuvData()), we have the more useful
return case of returning NULL.  So we're covered.

+        return false;
+    // Fallthrough.
+    case VideoFrame::YV16:
+      if (coded_size.width() % 2 != 0)

[Ami GONE FROM CHROMIUM, 2014/02/27 01:20:12]

I'd really like for this to be run through the "odd videos" test battery but I'm
not sure what that went.
The search is on in go/aqzkm

+        return false;
+    case VideoFrame::UNKNOWN:

[Ami GONE FROM CHROMIUM, 2014/02/27 01:20:12]

should be unreachable.

[sheu, 2014/02/28 00:40:19]

With the DCHECK in the VideoFrame constructor, these are reachable.

+    case VideoFrame::NATIVE_TEXTURE:
+    case VideoFrame::HISTOGRAM_MAX:
+#if defined(VIDEO_HOLE)
+    case VideoFrame::HOLE:
+#endif  // defined(VIDEO_HOLE)
+      break;
+  }
+
+  return true;
 }
 
 // static
@@ -129,6 +157,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 +196,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;
@@ -282,21 +314,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 +337,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 +348,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 +366,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 +449,13 @@ VideoFrame::VideoFrame(VideoFrame::Format format,
       shared_memory_handle_(base::SharedMemory::NULLHandle()),
       timestamp_(timestamp),
       end_of_stream_(end_of_stream) {
+  DCHECK(format_ == VideoFrame::UNKNOWN ||
+         IsValidConfig(format_, coded_size_, visible_rect_, natural_size_));
+  DCHECK_GE(visible_rect_.x(), 0);
+  DCHECK_GE(visible_rect_.y(), 0);
+  DCHECK_LE(visible_rect_.right(), coded_size_.width());
+  DCHECK_LE(visible_rect_.height(), coded_size_.height());

[Ami GONE FROM CHROMIUM, 2014/02/27 01:20:12]

These last 4 are part of IsValidConfig(), so why DCHECK them again?
Further, l.452 implies that being UNKNOWN makes it ok to not be IsValid, but
these 4 lines say you can be UNKNOWN and invalid, but you better not be invalid
in these 4 ways?!?

I suspect the only legit way to get here with UNKNOWN is CreateEOSFrame(), so
maybe just DCHECK for that?

[sheu, 2014/02/28 00:40:19]

Gonna fix up the check paths a bit.

+
   memset(&strides_, 0, sizeof(strides_));
   memset(&data_, 0, sizeof(data_));
 }