Plumb through cropped output size for VideoFrame

media/filters/skcanvas_video_renderer.cc

Index: media/filters/skcanvas_video_renderer.cc
diff --git a/media/filters/skcanvas_video_renderer.cc b/media/filters/skcanvas_video_renderer.cc
index bef015ef3a3f25ca6870a3def098cfa9a4ab5c1e..3b792bc6ac15c8506864416c8b8e64acacf9bc35 100644
--- a/media/filters/skcanvas_video_renderer.cc
+++ b/media/filters/skcanvas_video_renderer.cc
@@ -109,24 +109,26 @@ static void FastPaint(
   DCHECK_NE(0, dest_rect.width());
   DCHECK_NE(0, dest_rect.height());
   size_t frame_clip_width = local_dest_irect.width() *
-      video_frame->data_size().width() / local_dest_irect_saved.width();
+      video_frame->visible_rect().width() / local_dest_irect_saved.width();
   size_t frame_clip_height = local_dest_irect.height() *
-      video_frame->data_size().height() / local_dest_irect_saved.height();
+      video_frame->visible_rect().height() / local_dest_irect_saved.height();
 
   // Project the "left" and "top" of the final destination rect to local
   // coordinates of the video frame, use these values to find the offsets
   // in the video frame to start reading.
   size_t frame_clip_left =
+      video_frame->visible_rect().x() +
       (local_dest_irect.fLeft - local_dest_irect_saved.fLeft) *
-      video_frame->data_size().width() / local_dest_irect_saved.width();
+      video_frame->visible_rect().width() / local_dest_irect_saved.width();
   size_t frame_clip_top =
+      video_frame->visible_rect().y() +
       (local_dest_irect.fTop - local_dest_irect_saved.fTop) *
-      video_frame->data_size().height() / local_dest_irect_saved.height();
+      video_frame->visible_rect().height() / local_dest_irect_saved.height();

[Ami GONE FROM CHROMIUM, 2012/10/25 00:43:07]

Can you add testing for this stuff in the unittest for this file?

 
   // Use the "left" and "top" of the destination rect to locate the offset
   // in Y, U and V planes.
-  size_t y_offset = video_frame->stride(media::VideoFrame::kYPlane) *
-      frame_clip_top + frame_clip_left;
+  size_t y_offset = (video_frame->stride(media::VideoFrame::kYPlane) *
+                     frame_clip_top) + frame_clip_left;
 
   // For format YV12, there is one U, V value per 2x2 block.
   // For format YV16, there is one u, V value per 2x1 block.
@@ -139,7 +141,7 @@ static void FastPaint(
   uint8* frame_clip_v =
       video_frame->data(media::VideoFrame::kVPlane) + uv_offset;
 
-  // TODO(hclam): do rotation and mirroring here.
+  // TODO(hcfam): do rotation and mirroring here.

[Ami GONE FROM CHROMIUM, 2012/10/25 00:43:07]

lolwat

   // TODO(fbarchard): switch filtering based on performance.
   bitmap.lockPixels();
   media::ScaleYUVToRGB32(frame_clip_y,
@@ -174,26 +176,48 @@ static void ConvertVideoFrameToBitmap(
 
   // Check if |bitmap| needs to be (re)allocated.
   if (bitmap->isNull() ||
-      bitmap->width() != video_frame->data_size().width() ||
-      bitmap->height() != video_frame->data_size().height()) {
+      bitmap->width() != video_frame->visible_rect().width() ||
+      bitmap->height() != video_frame->visible_rect().height()) {
     bitmap->setConfig(SkBitmap::kARGB_8888_Config,
-                      video_frame->data_size().width(),
-                      video_frame->data_size().height());
+                      video_frame->visible_rect().width(),
+                      video_frame->visible_rect().height());
     bitmap->allocPixels();
     bitmap->setIsVolatile(true);
   }
 
+  media::YUVType yuv_type = (video_frame->format() == media::VideoFrame::YV12) ?

[Ami GONE FROM CHROMIUM, 2012/10/25 00:43:07]

This thunk is wrong for the native texture case, where none of it is relevant. 
I think you want to move it into the IsEitherYV12OrYV16 block at l.212

[sheu, 2012/10/25 01:53:54]

Done.

+                            media::YV12 : media::YV16;
+  int y_shift = yuv_type;  // 1 for YV12, 0 for YV16.
+
+  // Use the "left" and "top" of the destination rect to locate the offset
+  // in Y, U and V planes.
+  size_t y_offset = (video_frame->stride(media::VideoFrame::kYPlane) *
+                     video_frame->visible_rect().y()) +
+                    video_frame->visible_rect().x();
+
+  // For format YV12, there is one U, V value per 2x2 block.
+  // For format YV16, there is one u, V value per 2x1 block.
+  size_t uv_offset = (video_frame->stride(media::VideoFrame::kUPlane) *
+                      (video_frame->visible_rect().y() >> y_shift)) +
+                     (video_frame->visible_rect().x() >> 1);
+  uint8* frame_clip_y =
+      video_frame->data(media::VideoFrame::kYPlane) + y_offset;
+  uint8* frame_clip_u =
+      video_frame->data(media::VideoFrame::kUPlane) + uv_offset;
+  uint8* frame_clip_v =
+      video_frame->data(media::VideoFrame::kVPlane) + uv_offset;

[Ami GONE FROM CHROMIUM, 2012/10/25 00:43:07]

I think this thunk is correct (% needing to move per above), but it *definitely*
wants to be tested ;)

[sheu, 2012/10/25 01:53:54]

Done.

+
   bitmap->lockPixels();
   if (IsEitherYV12OrYV16(video_frame->format())) {
     media::YUVType yuv_type =
         (video_frame->format() == media::VideoFrame::YV12) ?
         media::YV12 : media::YV16;
-    media::ConvertYUVToRGB32(video_frame->data(media::VideoFrame::kYPlane),
-                             video_frame->data(media::VideoFrame::kUPlane),
-                             video_frame->data(media::VideoFrame::kVPlane),
+    media::ConvertYUVToRGB32(frame_clip_y,
+                             frame_clip_u,
+                             frame_clip_v,
                              static_cast<uint8*>(bitmap->getPixels()),
-                             video_frame->data_size().width(),
-                             video_frame->data_size().height(),
+                             video_frame->visible_rect().width(),
+                             video_frame->visible_rect().height(),
                              video_frame->stride(media::VideoFrame::kYPlane),
                              video_frame->stride(media::VideoFrame::kUPlane),
                              bitmap->rowBytes(),