Deduplicate YUV copying code, update call sites, and add some unit tests.

Historically such code is a source of out-of-bound reads/writes.

TEST=media_unittests

Review URL: http://codereview.chromium.org/7452009

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@93276 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 254 insertions, 59 deletions

diff --git a/media/base/video_frame.cc b/media/base/video_frame.cc
index 7d637db..c40c58f 100644
--- a/media/base/video_frame.cc
+++ b/media/base/video_frame.cc
@@ -9,26 +9,6 @@
 namespace media {
 
 // static
-size_t VideoFrame::GetNumberOfPlanes(VideoFrame::Format format) {
-  switch (format) {
-    case VideoFrame::RGB555:
-    case VideoFrame::RGB565:
-    case VideoFrame::RGB24:
-    case VideoFrame::RGB32:
-    case VideoFrame::RGBA:
-    case VideoFrame::ASCII:
-      return VideoFrame::kNumRGBPlanes;
-    case VideoFrame::YV12:
-    case VideoFrame::YV16:
-      return VideoFrame::kNumYUVPlanes;
-    case VideoFrame::NV12:
-      return VideoFrame::kNumNV12Planes;
-    default:
-      return 0;
-  }
-}
-
-// static
 scoped_refptr<VideoFrame> VideoFrame::CreateFrame(
     VideoFrame::Format format,
     size_t width,
@@ -172,6 +152,88 @@ VideoFrame::~VideoFrame() {
   delete[] data_[0];
 }
 
+bool VideoFrame::IsValidPlane(size_t plane) const {
+  switch (format_) {
+    case RGB555:
+    case RGB565:
+    case RGB24:
+    case RGB32:
+    case RGBA:
+      return plane == kRGBPlane;
+
+    case YV12:
+    case YV16:
+      return plane == kYPlane || plane == kUPlane || plane == kVPlane;
+
+    default:
+      break;
+  }
+
+  // Intentionally leave out non-production formats.
+  NOTREACHED() << "Unsupported video frame format: " << format_;
+  return false;
+}
+
+int VideoFrame::stride(size_t plane) const {
+  DCHECK(IsValidPlane(plane));
+  return strides_[plane];
+}
+
+int VideoFrame::row_bytes(size_t plane) const {
+  DCHECK(IsValidPlane(plane));
+  switch (format_) {
+    case RGB555:
+    case RGB565:
+    case RGB24:
+    case RGB32:
+    case RGBA:
+      return width_;
+
+    case YV12:
+    case YV16:
+      if (plane == kYPlane)
+        return width_;
+      return width_ / 2;
+
+    default:
+      break;
+  }
+
+  // Intentionally leave out non-production formats.
+  NOTREACHED() << "Unsupported video frame format: " << format_;
+  return 0;
+}
+
+int 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_;
+
+    case YV12:
+      if (plane == kYPlane)
+        return height_;
+      return height_ / 2;
+
+    default:
+      break;
+  }
+
+  // Intentionally leave out non-production formats.
+  NOTREACHED() << "Unsupported video frame format: " << format_;
+  return 0;
+}
+
+uint8* VideoFrame::data(size_t plane) const {
+  DCHECK(IsValidPlane(plane));
+  return data_[plane];
+}
+
 bool VideoFrame::IsEndOfStream() const {
   return format_ == VideoFrame::EMPTY;
 }
diff --git a/media/base/video_frame.h b/media/base/video_frame.h
index 684e522..529b9ee 100644
--- a/media/base/video_frame.h
+++ b/media/base/video_frame.h
@@ -40,9 +40,6 @@ class VideoFrame : public StreamSample {
     I420,        // 12bpp YVU planar 1x1 Y, 2x2 UV samples.
   };
 
-  // Get the number of planes for a video frame format.
-  static size_t GetNumberOfPlanes(VideoFrame::Format format);
-
   // Creates a new frame in system memory with given parameters. Buffers for
   // the frame are allocated but not initialized.
   static scoped_refptr<VideoFrame> CreateFrame(
@@ -68,11 +65,18 @@ class VideoFrame : public StreamSample {
 
   size_t planes() const { return planes_;  }
 
-  int32 stride(size_t plane) const { return strides_[plane]; }
+  int stride(size_t plane) const;
+
+  // Returns the number of bytes per row and number of rows for a given plane.
+  //
+  // As opposed to stride(), row_bytes() refers to the bytes representing
+  // visible pixels.
+  int row_bytes(size_t plane) const;
+  int rows(size_t plane) const;
 
   // Returns pointer to the buffer for a given plane. The memory is owned by
   // VideoFrame object and must not be freed by the caller.
-  uint8* data(size_t plane) const { return data_[plane]; }
+  uint8* data(size_t plane) const;
 
   // StreamSample interface.
   virtual bool IsEndOfStream() const;
@@ -89,6 +93,9 @@ class VideoFrame : public StreamSample {
   void AllocateRGB(size_t bytes_per_pixel);
   void AllocateYUV();
 
+  // Used to DCHECK() plane parameters.
+  bool IsValidPlane(size_t plane) const;
+
   // Frame format.
   Format format_;
 
diff --git a/media/base/video_util.cc b/media/base/video_util.cc
new file mode 100644
index 0000000..ed00667
--- /dev/null
+++ b/media/base/video_util.cc
@@ -0,0 +1,43 @@
+// Copyright (c) 2011 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_util.h"
+
+#include "base/logging.h"
+#include "media/base/video_frame.h"
+
+namespace media {
+
+static void CopyPlane(size_t plane, const uint8* source, int stride, int rows,
+                      VideoFrame* frame) {
+  uint8* dest = frame->data(plane);
+  int dest_stride = frame->stride(plane);
+
+  // Clamp in case source frame has smaller stride.
+  int bytes_to_copy_per_row = std::min(frame->row_bytes(plane), stride);
+
+  // Clamp in case source frame has smaller height.
+  int rows_to_copy = std::min(frame->rows(plane), rows);
+
+  // Copy!
+  for (int row = 0; row < rows_to_copy; ++row) {
+    memcpy(dest, source, bytes_to_copy_per_row);
+    source += stride;
+    dest += dest_stride;
+  }
+}
+
+void CopyYPlane(const uint8* source, int stride, int rows, VideoFrame* frame) {
+  CopyPlane(VideoFrame::kYPlane, source, stride, rows, frame);
+}
+
+void CopyUPlane(const uint8* source, int stride, int rows, VideoFrame* frame) {
+  CopyPlane(VideoFrame::kUPlane, source, stride, rows, frame);
+}
+
+void CopyVPlane(const uint8* source, int stride, int rows, VideoFrame* frame) {
+  CopyPlane(VideoFrame::kVPlane, source, stride, rows, frame);
+}
+
+}  // namespace media
diff --git a/media/base/video_util.h b/media/base/video_util.h
new file mode 100644
index 0000000..fa40984
--- /dev/null
+++ b/media/base/video_util.h
@@ -0,0 +1,24 @@
+// Copyright (c) 2011 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.
+
+#ifndef MEDIA_BASE_VIDEO_UTIL_H_
+#define MEDIA_BASE_VIDEO_UTIL_H_
+
+#include "base/basictypes.h"
+
+namespace media {
+
+class VideoFrame;
+
+// Copies a plane of YUV source into a VideoFrame object, taking into account
+// source and destinations dimensions.
+//
+// NOTE: rows is *not* the same as height!
+void CopyYPlane(const uint8* source, int stride, int rows, VideoFrame* frame);
+void CopyUPlane(const uint8* source, int stride, int rows, VideoFrame* frame);
+void CopyVPlane(const uint8* source, int stride, int rows, VideoFrame* frame);
+
+}  // namespace media
+
+#endif  // MEDIA_BASE_VIDEO_UTIL_H_
diff --git a/media/base/video_util_unittest.cc b/media/base/video_util_unittest.cc
new file mode 100644
index 0000000..a869671
--- /dev/null
+++ b/media/base/video_util_unittest.cc
@@ -0,0 +1,84 @@
+// Copyright (c) 2011 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 "base/memory/scoped_ptr.h"
+#include "media/base/video_frame.h"
+#include "media/base/video_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace media {
+
+class VideoUtilTest : public testing::Test {
+ public:
+  VideoUtilTest()
+      : height_(0),
+        y_stride_(0),
+        u_stride_(0),
+        v_stride_(0) {
+  }
+
+  virtual ~VideoUtilTest() {}
+
+  void CreateSourceFrame(int width, int height,
+                         int y_stride, int u_stride, int v_stride) {
+    EXPECT_GE(y_stride, width);
+    EXPECT_GE(u_stride, width / 2);
+    EXPECT_GE(v_stride, width / 2);
+
+    height_ = height;
+    y_stride_ = y_stride;
+    u_stride_ = u_stride;
+    v_stride_ = v_stride;
+
+    y_plane_.reset(new uint8[y_stride * height]);
+    u_plane_.reset(new uint8[u_stride * height / 2]);
+    v_plane_.reset(new uint8[v_stride * height / 2]);
+  }
+
+  void CreateDestinationFrame(int width, int height) {
+    destination_frame_ =
+        VideoFrame::CreateFrame(VideoFrame::YV12, width, height,
+                                base::TimeDelta(), base::TimeDelta());
+  }
+
+  void CopyPlanes() {
+    CopyYPlane(y_plane_.get(), y_stride_, height_, destination_frame_);
+    CopyUPlane(u_plane_.get(), u_stride_, height_ / 2, destination_frame_);
+    CopyVPlane(v_plane_.get(), v_stride_, height_ / 2, destination_frame_);
+  }
+
+ private:
+  scoped_array<uint8> y_plane_;
+  scoped_array<uint8> u_plane_;
+  scoped_array<uint8> v_plane_;
+
+  int height_;
+  int y_stride_;
+  int u_stride_;
+  int v_stride_;
+
+  scoped_refptr<VideoFrame> destination_frame_;
+
+  DISALLOW_COPY_AND_ASSIGN(VideoUtilTest);
+};
+
+TEST_F(VideoUtilTest, CopyPlane_Exact) {
+  CreateSourceFrame(16, 16, 16, 8, 8);
+  CreateDestinationFrame(16, 16);
+  CopyPlanes();
+}
+
+TEST_F(VideoUtilTest, CopyPlane_SmallerSource) {
+  CreateSourceFrame(8, 8, 8, 4, 4);
+  CreateDestinationFrame(16, 16);
+  CopyPlanes();
+}
+
+TEST_F(VideoUtilTest, CopyPlane_SmallerDestination) {
+  CreateSourceFrame(16, 16, 16, 8, 8);
+  CreateDestinationFrame(8, 8);
+  CopyPlanes();
+}
+
+}  // namespace media
diff --git a/media/media.gyp b/media/media.gyp
index 7c3d16f..9809a7e 100644
--- a/media/media.gyp
+++ b/media/media.gyp
@@ -130,6 +130,8 @@
         'base/video_decoder_config.h',
         'base/video_frame.cc',
         'base/video_frame.h',
+        'base/video_util.cc',
+        'base/video_util.h',
         'ffmpeg/ffmpeg_common.cc',
         'ffmpeg/ffmpeg_common.h',
         'ffmpeg/file_protocol.cc',
@@ -423,6 +425,7 @@
         'base/seekable_buffer_unittest.cc',
         'base/state_matrix_unittest.cc',
         'base/video_frame_unittest.cc',
+        'base/video_util_unittest.cc',
         'base/yuv_convert_unittest.cc',
         'ffmpeg/ffmpeg_common_unittest.cc',
         'filters/adaptive_demuxer_unittest.cc',
diff --git a/media/video/ffmpeg_video_decode_engine.cc b/media/video/ffmpeg_video_decode_engine.cc
index ce37a7d..45319f4 100644
--- a/media/video/ffmpeg_video_decode_engine.cc
+++ b/media/video/ffmpeg_video_decode_engine.cc
@@ -12,6 +12,7 @@
 #include "media/base/limits.h"
 #include "media/base/media_switches.h"
 #include "media/base/pipeline.h"
+#include "media/base/video_util.h"
 #include "media/ffmpeg/ffmpeg_common.h"
 #include "media/filters/ffmpeg_demuxer.h"
 
@@ -130,36 +131,6 @@ void FFmpegVideoDecodeEngine::Initialize(
   event_handler_->OnInitializeComplete(info);
 }
 
-// TODO(scherkus): Move this function to a utility class and unit test.
-static void CopyPlane(size_t plane,
-                      scoped_refptr<VideoFrame> video_frame,
-                      const AVFrame* frame,
-                      size_t source_height) {
-  DCHECK_EQ(video_frame->width() % 2, 0u);
-  const uint8* source = frame->data[plane];
-  const size_t source_stride = frame->linesize[plane];
-  uint8* dest = video_frame->data(plane);
-  const size_t dest_stride = video_frame->stride(plane);
-
-  // Calculate amounts to copy and clamp to minium frame dimensions.
-  size_t bytes_per_line = video_frame->width();
-  size_t copy_lines = std::min(video_frame->height(), source_height);
-  if (plane != VideoFrame::kYPlane) {
-    bytes_per_line /= 2;
-    if (video_frame->format() == VideoFrame::YV12) {
-      copy_lines = (copy_lines + 1) / 2;
-    }
-  }
-  bytes_per_line = std::min(bytes_per_line, source_stride);
-
-  // Copy!
-  for (size_t i = 0; i < copy_lines; ++i) {
-    memcpy(dest, source, bytes_per_line);
-    source += source_stride;
-    dest += dest_stride;
-  }
-}
-
 void FFmpegVideoDecodeEngine::ConsumeVideoSample(
     scoped_refptr<Buffer> buffer) {
   pending_input_buffers_--;
@@ -280,10 +251,11 @@ void FFmpegVideoDecodeEngine::DecodeFrame(scoped_refptr<Buffer> buffer) {
   // Copy the frame data since FFmpeg reuses internal buffers for AVFrame
   // output, meaning the data is only valid until the next
   // avcodec_decode_video() call.
-  size_t height = codec_context_->height;
-  CopyPlane(VideoFrame::kYPlane, video_frame.get(), av_frame_.get(), height);
-  CopyPlane(VideoFrame::kUPlane, video_frame.get(), av_frame_.get(), height);
-  CopyPlane(VideoFrame::kVPlane, video_frame.get(), av_frame_.get(), height);
+  int y_rows = codec_context_->height;
+  int uv_rows = codec_context_->height / 2;
+  CopyYPlane(av_frame_->data[0], av_frame_->linesize[0], y_rows, video_frame);
+  CopyUPlane(av_frame_->data[1], av_frame_->linesize[1], uv_rows, video_frame);
+  CopyVPlane(av_frame_->data[2], av_frame_->linesize[2], uv_rows, video_frame);
 
   video_frame->SetTimestamp(timestamp);
   video_frame->SetDuration(duration);