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

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 <algorithm>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/memory/aligned_memory.h"
 #include "base/strings/string_piece.h"
 #include "gpu/command_buffer/common/mailbox_holder.h"
 #include "media/base/limits.h"
 #include "media/base/video_util.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
 namespace media {
 
 // static
 scoped_refptr<VideoFrame> VideoFrame::CreateFrame(
     VideoFrame::Format format,
     const gfx::Size& coded_size,
     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;
 }
 
 // static
 std::string VideoFrame::FormatToString(VideoFrame::Format format) {
   switch (format) {
     case VideoFrame::UNKNOWN:
       return "UNKNOWN";
     case VideoFrame::YV12:
       return "YV12";
@@ skipping 16 matching lines @@
   }
   NOTREACHED() << "Invalid videoframe format provided: " << format;
   return "";
 }
 
 // static
 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
 scoped_refptr<VideoFrame> VideoFrame::WrapNativeTexture(
     scoped_ptr<gpu::MailboxHolder> mailbox_holder,
     const ReleaseMailboxCB& mailbox_holder_release_cb,
     const gfx::Size& coded_size,
     const gfx::Rect& visible_rect,
     const gfx::Size& natural_size,
     base::TimeDelta timestamp,
@@ skipping 21 matching lines @@
 scoped_refptr<VideoFrame> VideoFrame::WrapExternalPackedMemory(
     Format format,
     const gfx::Size& coded_size,
     const gfx::Rect& visible_rect,
     const gfx::Size& natural_size,
     uint8* data,
     size_t data_size,
     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;
 
   switch (format) {
     case I420: {
       scoped_refptr<VideoFrame> frame(new VideoFrame(
           format, coded_size, visible_rect, natural_size, timestamp, false));
       frame->shared_memory_handle_ = handle;
       frame->strides_[kYPlane] = coded_size.width();
       frame->strides_[kUPlane] = coded_size.width() / 2;
@@ skipping 17 matching lines @@
     const gfx::Rect& visible_rect,
     const gfx::Size& natural_size,
     int32 y_stride,
     int32 u_stride,
     int32 v_stride,
     uint8* y_data,
     uint8* u_data,
     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;
   frame->strides_[kUPlane] = u_stride;
   frame->strides_[kVPlane] = v_stride;
   frame->data_[kYPlane] = y_data;
   frame->data_[kUPlane] = u_data;
   frame->data_[kVPlane] = v_data;
   frame->no_longer_needed_cb_ = no_longer_needed_cb;
   return frame;
@@ skipping 95 matching lines @@
 
 // static
 size_t VideoFrame::AllocationSize(Format format, const gfx::Size& coded_size) {
   size_t total = 0;
   for (size_t i = 0; i < NumPlanes(format); ++i)
     total += PlaneAllocationSize(format, i, coded_size);
   return total;
 }
 
 // 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;
       }
     }
     case VideoFrame::YV12A: {
       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;
       }
     }
     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;
       }
     }
     case VideoFrame::UNKNOWN:
     case VideoFrame::NATIVE_TEXTURE:
     case VideoFrame::HISTOGRAM_MAX:
 #if defined(VIDEO_HOLE)
     case VideoFrame::HOLE:
 #endif  // defined(VIDEO_HOLE)
       break;
   }
   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().
 static void ReleaseData(uint8* data) {
   DCHECK(data);
   base::AlignedFree(data);
 }
 
 void VideoFrame::AllocateYUV() {
   DCHECK(format_ == VideoFrame::YV12 || format_ == VideoFrame::YV16 ||
@@ skipping 55 matching lines @@
                        const gfx::Size& natural_size,
                        base::TimeDelta timestamp,
                        bool end_of_stream)
     : format_(format),
       coded_size_(coded_size),
       visible_rect_(visible_rect),
       natural_size_(natural_size),
       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_));
 }
 
 VideoFrame::~VideoFrame() {
   if (!mailbox_holder_release_cb_.is_null()) {
     base::ResetAndReturn(&mailbox_holder_release_cb_)
         .Run(mailbox_holder_.Pass());
   }
   if (!no_longer_needed_cb_.is_null())
@@ skipping 81 matching lines @@
       break;
     for (int row = 0; row < rows(plane); ++row) {
       base::MD5Update(context, base::StringPiece(
           reinterpret_cast<char*>(data(plane) + stride(plane) * row),
           row_bytes(plane)));
     }
   }
 }
 
 }  // namespace media