Allow odd sized video decoder configs.

VideoFrame::IsValidConfig is a public static method, called both externally and internally. This change removes a check which is not wanted in all (or even most) cases where IsValidConfig was called. Spoke with some past authors. The check is not well understood, but best guess is that it was meant to verify that coded size is properly aligned with the sub-sampled UV planes. The check is now made more narrowly in CreateFrame, where we are free to adjust the size because we own the allocation. This change also includes some unification of error handling among the various "Create" methods. These will now return null if the video frame configuration is found to be invalid. See extensive discussion and back-story in code review comments. BUG=505070, 489744, 379127 TESTS=media_unittests (added some), and manually tested repro in bug Review URL: https://codereview.chromium.org/1240833003 Cr-Commit-Position: refs/heads/master@{#340946}
author: chcunningham <chcunningham@chromium.org> 2015-07-29 12:23:22 -0700
committer: Commit bot <commit-bot@chromium.org> 2015-07-29 19:23:50 +0000
commit: af2caf7f5bef24e3db84c95d7d8d1203bf050e27 (patch)
tree: 551f6300c5d36784194234b700536e7aad5002bd /media/base
parent: 5af02f56a271ef100cfd8abfe1079b4a35534e2c (diff)
download: chromium_src-af2caf7f5bef24e3db84c95d7d8d1203bf050e27.zip
chromium_src-af2caf7f5bef24e3db84c95d7d8d1203bf050e27.tar.gz
chromium_src-af2caf7f5bef24e3db84c95d7d8d1203bf050e27.tar.bz2
3 files changed, 174 insertions, 94 deletions
diff --git a/media/base/video_frame.cc b/media/base/video_frame.cc
index 51269a8..f80f9aa 100644
--- a/media/base/video_frame.cc
+++ b/media/base/video_frame.cc
@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/memory/aligned_memory.h"
 #include "base/strings/string_piece.h"
+#include "base/strings/stringprintf.h"
 #include "media/base/limits.h"
 #include "media/base/video_util.h"
 #include "ui/gfx/geometry/point.h"
@@ -31,6 +32,17 @@ static inline size_t RoundDown(size_t value, size_t alignment) {
   return value & ~(alignment - 1);
 }
 
+static std::string ConfigToString(const VideoPixelFormat format,
+                                  const VideoFrame::StorageType storage_type,
+                                  const gfx::Size& coded_size,
+                                  const gfx::Rect& visible_rect,
+                                  const gfx::Size& natural_size) {
+  return base::StringPrintf(
+      "format:%s coded_size:%s visible_rect:%s natural_size:%s",
+      VideoPixelFormatToString(format).c_str(), coded_size.ToString().c_str(),
+      visible_rect.ToString().c_str(), natural_size.ToString().c_str());
+}
+
 // Returns true if |plane| is a valid plane index for the given |format|.
 static bool IsValidPlane(size_t plane, VideoPixelFormat format) {
   DCHECK_LE(VideoFrame::NumPlanes(format),
@@ -116,14 +128,6 @@ static int BytesPerElement(VideoPixelFormat format, size_t plane) {
   return 1;
 }
 
-// Rounds up |coded_size| if necessary for |format|.
-static gfx::Size AdjustCodedSize(VideoPixelFormat format,
-                                 const gfx::Size& coded_size) {
-  const gfx::Size alignment = CommonAlignment(format);
-  return gfx::Size(RoundUp(coded_size.width(), alignment.width()),
-                   RoundUp(coded_size.height(), alignment.height()));
-}
-
 // static
 bool VideoFrame::IsValidConfig(VideoPixelFormat format,
                                StorageType storage_type,
@@ -147,36 +151,18 @@ bool VideoFrame::IsValidConfig(VideoPixelFormat format,
   if (!IsStorageTypeMappable(storage_type))
     return true;
 
-  // Check format-specific width/height requirements.
-  switch (format) {
-    case PIXEL_FORMAT_UNKNOWN:
-      return (coded_size.IsEmpty() && visible_rect.IsEmpty() &&
-              natural_size.IsEmpty());
-    case PIXEL_FORMAT_YV24:
-    case PIXEL_FORMAT_YV12:
-    case PIXEL_FORMAT_I420:
-    case PIXEL_FORMAT_YV12A:
-    case PIXEL_FORMAT_YV16:
-    case PIXEL_FORMAT_ARGB:
-    case PIXEL_FORMAT_XRGB:
-#if defined(OS_MACOSX) || defined(OS_CHROMEOS)
-    case PIXEL_FORMAT_NV12:
-#endif
-      // Check that software-allocated buffer formats are aligned correctly and
-      // not empty.
-      const gfx::Size alignment = CommonAlignment(format);
-      return RoundUp(visible_rect.right(), alignment.width()) <=
-                 static_cast<size_t>(coded_size.width()) &&
-             RoundUp(visible_rect.bottom(), alignment.height()) <=
-                 static_cast<size_t>(coded_size.height()) &&
-             !coded_size.IsEmpty() && !visible_rect.IsEmpty() &&
-             !natural_size.IsEmpty();
+  // Make sure new formats are properly accounted for in the method.
+  static_assert(PIXEL_FORMAT_MAX == 8,
+                "Added pixel format, please review IsValidConfig()");
+
+  if (format == PIXEL_FORMAT_UNKNOWN) {
+    return coded_size.IsEmpty() && visible_rect.IsEmpty() &&
+           natural_size.IsEmpty();
   }
 
-  // TODO(mcasas): Check that storage type and underlying mailboxes/dataptr are
-  // matching.
-  NOTREACHED();
-  return false;
+  // Check that software-allocated buffer formats are not empty.
+  return !coded_size.IsEmpty() && !visible_rect.IsEmpty() &&
+         !natural_size.IsEmpty();
 }
 
 // static
@@ -192,14 +178,26 @@ scoped_refptr<VideoFrame> VideoFrame::CreateFrame(VideoPixelFormat format,
 
   // 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.
-  const gfx::Size new_coded_size = AdjustCodedSize(format, coded_size);
-  DCHECK(IsValidConfig(format, STORAGE_OWNED_MEMORY, new_coded_size,
-                       visible_rect, natural_size));
-
-  scoped_refptr<VideoFrame> frame(new VideoFrame(format, STORAGE_OWNED_MEMORY,
-                                                 new_coded_size, visible_rect,
-                                                 natural_size, timestamp));
+  // request does not line up on sample boundaries. See discussion at
+  // http://crrev.com/1240833003
+  const gfx::Size alignment = CommonAlignment(format);
+  const gfx::Size new_coded_size =
+      gfx::Size(RoundUp(coded_size.width(), alignment.width()),
+                RoundUp(coded_size.height(), alignment.height()));
+  DCHECK((new_coded_size.width() % alignment.width() == 0) &&
+         (new_coded_size.height() % alignment.height() == 0));
+
+  const StorageType storage = STORAGE_OWNED_MEMORY;
+  if (!IsValidConfig(format, storage, new_coded_size, visible_rect,
+                     natural_size)) {
+    DLOG(ERROR) << __FUNCTION__ << " Invalid config."
+                << ConfigToString(format, storage, coded_size, visible_rect,
+                                  natural_size);
+    return nullptr;
+  }
+
+  scoped_refptr<VideoFrame> frame(new VideoFrame(
+      format, storage, new_coded_size, visible_rect, natural_size, timestamp));
   frame->AllocateYUV();
   return frame;
 }
@@ -218,11 +216,18 @@ scoped_refptr<VideoFrame> VideoFrame::WrapNativeTexture(
                 << VideoPixelFormatToString(format);
     return nullptr;
   }
+  const StorageType storage = STORAGE_OPAQUE;
+  if (!IsValidConfig(format, storage, coded_size, visible_rect, natural_size)) {
+    DLOG(ERROR) << __FUNCTION__ << " Invalid config."
+                << ConfigToString(format, storage, coded_size, visible_rect,
+                                  natural_size);
+    return nullptr;
+  }
+
   gpu::MailboxHolder mailbox_holders[kMaxPlanes];
   mailbox_holders[kARGBPlane] = mailbox_holder;
-  return new VideoFrame(format, STORAGE_OPAQUE, coded_size,
-                        visible_rect, natural_size, mailbox_holders,
-                        mailbox_holder_release_cb, timestamp);
+  return new VideoFrame(format, storage, coded_size, visible_rect, natural_size,
+                        mailbox_holders, mailbox_holder_release_cb, timestamp);
 }
 
 // static
@@ -235,13 +240,21 @@ scoped_refptr<VideoFrame> VideoFrame::WrapYUV420NativeTextures(
     const gfx::Rect& visible_rect,
     const gfx::Size& natural_size,
     base::TimeDelta timestamp) {
+  const StorageType storage = STORAGE_OPAQUE;
+  VideoPixelFormat format = PIXEL_FORMAT_I420;
+  if (!IsValidConfig(format, storage, coded_size, visible_rect, natural_size)) {
+    DLOG(ERROR) << __FUNCTION__ << " Invalid config."
+                << ConfigToString(format, storage, coded_size, visible_rect,
+                                  natural_size);
+    return nullptr;
+  }
+
   gpu::MailboxHolder mailbox_holders[kMaxPlanes];
   mailbox_holders[kYPlane] = y_mailbox_holder;
   mailbox_holders[kUPlane] = u_mailbox_holder;
   mailbox_holders[kVPlane] = v_mailbox_holder;
-  return new VideoFrame(PIXEL_FORMAT_I420, STORAGE_OPAQUE, coded_size,
-                        visible_rect, natural_size, mailbox_holders,
-                        mailbox_holder_release_cb, timestamp);
+  return new VideoFrame(format, storage, coded_size, visible_rect, natural_size,
+                        mailbox_holders, mailbox_holder_release_cb, timestamp);
 }
 
 // static
@@ -287,13 +300,16 @@ scoped_refptr<VideoFrame> VideoFrame::WrapExternalYuvData(
     uint8* u_data,
     uint8* v_data,
     base::TimeDelta timestamp) {
-  const gfx::Size new_coded_size = AdjustCodedSize(format, coded_size);
-  CHECK(IsValidConfig(format, STORAGE_UNOWNED_MEMORY, new_coded_size,
-                      visible_rect, natural_size));
+  const StorageType storage = STORAGE_UNOWNED_MEMORY;
+  if (!IsValidConfig(format, storage, coded_size, visible_rect, natural_size)) {
+    DLOG(ERROR) << __FUNCTION__ << " Invalid config."
+                << ConfigToString(format, storage, coded_size, visible_rect,
+                                  natural_size);
+    return nullptr;
+  }
 
-  scoped_refptr<VideoFrame> frame(new VideoFrame(format, STORAGE_UNOWNED_MEMORY,
-                                                 new_coded_size, visible_rect,
-                                                 natural_size, timestamp));
+  scoped_refptr<VideoFrame> frame(new VideoFrame(
+      format, storage, coded_size, visible_rect, natural_size, timestamp));
   frame->strides_[kYPlane] = y_stride;
   frame->strides_[kUPlane] = u_stride;
   frame->strides_[kVPlane] = v_stride;
@@ -316,15 +332,18 @@ scoped_refptr<VideoFrame> VideoFrame::WrapExternalDmabufs(
   DCHECK_EQ(format, PIXEL_FORMAT_NV12);
 #endif
 
-  if (!IsValidConfig(format, STORAGE_DMABUFS, coded_size, visible_rect,
-                     natural_size)) {
+  const StorageType storage = STORAGE_DMABUFS;
+  if (!IsValidConfig(format, storage, coded_size, visible_rect, natural_size)) {
+    DLOG(ERROR) << __FUNCTION__ << " Invalid config."
+                << ConfigToString(format, storage, coded_size, visible_rect,
+                                  natural_size);
     return nullptr;
   }
+
   gpu::MailboxHolder mailbox_holders[kMaxPlanes];
   scoped_refptr<VideoFrame> frame =
-      new VideoFrame(format, STORAGE_DMABUFS, coded_size, visible_rect,
-                     natural_size, mailbox_holders, ReleaseMailboxCB(),
-                     timestamp);
+      new VideoFrame(format, storage, coded_size, visible_rect, natural_size,
+                     mailbox_holders, ReleaseMailboxCB(), timestamp);
   if (!frame || !frame->DuplicateFileDescriptors(dmabuf_fds))
     return nullptr;
   return frame;
@@ -352,21 +371,23 @@ scoped_refptr<VideoFrame> VideoFrame::WrapCVPixelBuffer(
     format = PIXEL_FORMAT_NV12;
   } else {
     DLOG(ERROR) << "CVPixelBuffer format not supported: " << cv_format;
-    return NULL;
+    return nullptr;
   }
 
   const gfx::Size coded_size(CVImageBufferGetEncodedSize(cv_pixel_buffer));
   const gfx::Rect visible_rect(CVImageBufferGetCleanRect(cv_pixel_buffer));
   const gfx::Size natural_size(CVImageBufferGetDisplaySize(cv_pixel_buffer));
+  const StorageType storage = STORAGE_UNOWNED_MEMORY;
 
-  if (!IsValidConfig(format, STORAGE_UNOWNED_MEMORY,
-                     coded_size, visible_rect, natural_size)) {
-    return NULL;
+  if (!IsValidConfig(format, storage, coded_size, visible_rect, natural_size)) {
+    DLOG(ERROR) << __FUNCTION__ << " Invalid config."
+                << ConfigToString(format, storage, coded_size, visible_rect,
+                                  natural_size);
+    return nullptr;
   }
 
-  scoped_refptr<VideoFrame> frame(new VideoFrame(format, STORAGE_UNOWNED_MEMORY,
-                                                 coded_size, visible_rect,
-                                                 natural_size, timestamp));
+  scoped_refptr<VideoFrame> frame(new VideoFrame(
+      format, storage, coded_size, visible_rect, natural_size, timestamp));
 
   frame->cv_pixel_buffer_.reset(cv_pixel_buffer, base::scoped_policy::RETAIN);
   return frame;
@@ -383,6 +404,16 @@ scoped_refptr<VideoFrame> VideoFrame::WrapVideoFrame(
   CHECK(!frame->HasTextures());
 
   DCHECK(frame->visible_rect().Contains(visible_rect));
+
+  if (!IsValidConfig(frame->format(), frame->storage_type(),
+                     frame->coded_size(), visible_rect, natural_size)) {
+    DLOG(ERROR) << __FUNCTION__ << " Invalid config."
+                << ConfigToString(frame->format(), frame->storage_type(),
+                                  frame->coded_size(), visible_rect,
+                                  natural_size);
+    return nullptr;
+  }
+
   scoped_refptr<VideoFrame> wrapping_frame(new VideoFrame(
       frame->format(), frame->storage_type(), frame->coded_size(), visible_rect,
       natural_size, frame->timestamp()));
@@ -459,11 +490,16 @@ scoped_refptr<VideoFrame> VideoFrame::CreateTransparentFrame(
 // static
 scoped_refptr<VideoFrame> VideoFrame::CreateHoleFrame(
     const gfx::Size& size) {
-  DCHECK(IsValidConfig(PIXEL_FORMAT_UNKNOWN, STORAGE_HOLE, size,
-                       gfx::Rect(size), size));
-  scoped_refptr<VideoFrame> frame(
-      new VideoFrame(PIXEL_FORMAT_UNKNOWN, STORAGE_HOLE, size, gfx::Rect(size),
-                     size, base::TimeDelta()));
+  const VideoPixelFormat format = PIXEL_FORMAT_UNKNOWN;
+  const StorageType storage = STORAGE_HOLE;
+  const gfx::Rect visible_rect = gfx::Rect(size);
+  if (!IsValidConfig(format, storage, size, visible_rect, size)) {
+    DLOG(ERROR) << __FUNCTION__ << " Invalid config."
+                << ConfigToString(format, storage, size, visible_rect, size);
+    return nullptr;
+  }
+  scoped_refptr<VideoFrame> frame(new VideoFrame(
+      format, storage, size, gfx::Rect(size), size, base::TimeDelta()));
   return frame;
 }
 #endif  // defined(VIDEO_HOLE)
@@ -727,33 +763,34 @@ scoped_refptr<VideoFrame> VideoFrame::WrapExternalStorage(
     size_t data_offset) {
   DCHECK(IsStorageTypeMappable(storage_type));
 
-  const gfx::Size new_coded_size = AdjustCodedSize(format, coded_size);
-  if (!IsValidConfig(format, storage_type, new_coded_size, visible_rect,
-                     natural_size) ||
-      data_size < AllocationSize(format, new_coded_size)) {
-    return NULL;
+  if (format != PIXEL_FORMAT_I420) {
+    DLOG(ERROR) << "Only PIXEL_FORMAT_I420 format supported: "
+                << VideoPixelFormatToString(format);
+    return nullptr;
+  }
+
+  if (!IsValidConfig(format, storage_type, coded_size, visible_rect,
+                     natural_size)) {
+    DLOG(ERROR) << __FUNCTION__ << " Invalid config."
+                << ConfigToString(format, storage_type, coded_size,
+                                  visible_rect, natural_size);
+    return nullptr;
   }
-  DLOG_IF(ERROR, format != PIXEL_FORMAT_I420)
-      << "Only PIXEL_FORMAT_I420 format supported: "
-      << VideoPixelFormatToString(format);
-  if (format != PIXEL_FORMAT_I420)
-    return NULL;
 
   scoped_refptr<VideoFrame> frame;
   if (storage_type == STORAGE_SHMEM) {
-    frame = new VideoFrame(format, storage_type, new_coded_size,
-                           visible_rect, natural_size, timestamp, handle,
-                           data_offset);
+    frame = new VideoFrame(format, storage_type, coded_size, visible_rect,
+                           natural_size, timestamp, handle, data_offset);
   } else {
-    frame = new VideoFrame(format, storage_type, new_coded_size,
-                           visible_rect, natural_size, timestamp);
+    frame = new VideoFrame(format, storage_type, coded_size, visible_rect,
+                           natural_size, timestamp);
   }
-  frame->strides_[kYPlane] = new_coded_size.width();
-  frame->strides_[kUPlane] = new_coded_size.width() / 2;
-  frame->strides_[kVPlane] = new_coded_size.width() / 2;
+  frame->strides_[kYPlane] = coded_size.width();
+  frame->strides_[kUPlane] = coded_size.width() / 2;
+  frame->strides_[kVPlane] = coded_size.width() / 2;
   frame->data_[kYPlane] = data;
-  frame->data_[kUPlane] = data + new_coded_size.GetArea();
-  frame->data_[kVPlane] = data + (new_coded_size.GetArea() * 5 / 4);
+  frame->data_[kUPlane] = data + coded_size.GetArea();
+  frame->data_[kVPlane] = data + (coded_size.GetArea() * 5 / 4);
   return frame;
 }
 
diff --git a/media/base/video_frame.h b/media/base/video_frame.h
index bc92b9a..80169cb 100644
--- a/media/base/video_frame.h
+++ b/media/base/video_frame.h
@@ -96,7 +96,6 @@ class MEDIA_EXPORT VideoFrame : public base::RefCountedThreadSafe<VideoFrame> {
 
   // Call prior to CreateFrame to ensure validity of frame configuration. Called
   // automatically by VideoDecoderConfig::IsValidConfig().
-  // TODO(scherkus): VideoDecoderConfig shouldn't call this method
   static bool IsValidConfig(VideoPixelFormat format,
                             StorageType storage_type,
                             const gfx::Size& coded_size,
diff --git a/media/base/video_frame_unittest.cc b/media/base/video_frame_unittest.cc
index 1096766..f1afcc8 100644
--- a/media/base/video_frame_unittest.cc
+++ b/media/base/video_frame_unittest.cc
@@ -330,6 +330,50 @@ TEST(VideoFrame,
   EXPECT_EQ(release_sync_point, called_sync_point);
 }
 
+TEST(VideoFrame, IsValidConfig_OddCodedSize) {
+  // Odd sizes are valid for all formats. Odd formats may be internally rounded
+  // in VideoFrame::CreateFrame because VideoFrame owns the allocation and can
+  // pad the requested coded_size to ensure the UV sample boundaries line up
+  // with the Y plane after subsample scaling. See CreateFrame_OddWidth.
+  gfx::Size odd_size(677, 288);
+
+  // First choosing a format with sub-sampling for UV.
+  EXPECT_TRUE(VideoFrame::IsValidConfig(
+      PIXEL_FORMAT_I420, VideoFrame::STORAGE_OWNED_MEMORY, odd_size,
+      gfx::Rect(odd_size), odd_size));
+
+  // Next try a format with no sub-sampling for UV.
+  EXPECT_TRUE(VideoFrame::IsValidConfig(
+      PIXEL_FORMAT_YV24, VideoFrame::STORAGE_OWNED_MEMORY, odd_size,
+      gfx::Rect(odd_size), odd_size));
+}
+
+TEST(VideoFrame, CreateFrame_OddWidth) {
+  // Odd sizes are non-standard for YUV formats that subsample the UV, but they
+  // do exist in the wild and should be gracefully handled by VideoFrame in
+  // situations where VideoFrame allocates the YUV memory. See discussion in
+  // crrev.com/1240833003
+  const gfx::Size odd_size(677, 288);
+  const base::TimeDelta kTimestamp = base::TimeDelta();
+
+  // First create a frame that sub-samples UV.
+  scoped_refptr<VideoFrame> frame = VideoFrame::CreateFrame(
+      PIXEL_FORMAT_I420, odd_size, gfx::Rect(odd_size), odd_size, kTimestamp);
+  ASSERT_TRUE(frame.get());
+  // I420 aligns UV to every 2 Y pixels. Hence, 677 should be rounded to 678
+  // which is the nearest value such that width % 2 == 0
+  EXPECT_EQ(678, frame->coded_size().width());
+
+  // Next create a frame that does not sub-sample UV.
+  frame = VideoFrame::CreateFrame(PIXEL_FORMAT_YV24, odd_size,
+                                  gfx::Rect(odd_size), odd_size, kTimestamp);
+  ASSERT_TRUE(frame.get());
+  // No sub-sampling for YV24 will mean odd width can remain odd since any pixel
+  // in the Y plane has a a corresponding pixel in the UV planes at the same
+  // index.
+  EXPECT_EQ(677, frame->coded_size().width());
+}
+
 TEST(VideoFrameMetadata, SetAndThenGetAllKeysForAllTypes) {
   VideoFrameMetadata metadata;
author	chcunningham <chcunningham@chromium.org>	2015-07-29 12:23:22 -0700
committer	Commit bot <commit-bot@chromium.org>	2015-07-29 19:23:50 +0000
commit	af2caf7f5bef24e3db84c95d7d8d1203bf050e27 (patch)
tree	551f6300c5d36784194234b700536e7aad5002bd /media/base
parent	5af02f56a271ef100cfd8abfe1079b4a35534e2c (diff)
download	chromium_src-af2caf7f5bef24e3db84c95d7d8d1203bf050e27.zip chromium_src-af2caf7f5bef24e3db84c95d7d8d1203bf050e27.tar.gz chromium_src-af2caf7f5bef24e3db84c95d7d8d1203bf050e27.tar.bz2