Plumb through cropped output size for VideoFrame

media/filters/skcanvas_video_renderer.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/filters/skcanvas_video_renderer.h"
 
 #include "base/logging.h"
 #include "media/base/video_frame.h"
 #include "media/base/yuv_convert.h"
 #include "third_party/skia/include/core/SkCanvas.h"
@@ skipping 91 matching lines @@
   // Project the clip rect to the original video frame, obtains the
   // dimensions of the projected clip rect, "left" and "top" of the rect.
   // The math here are all integer math so we won't have rounding error and
   // write outside of the canvas.
   // We have the assumptions of dest_rect.width() and dest_rect.height()
   // being non-zero, these are valid assumptions since finding intersection
   // above rejects empty rectangle so we just do a DCHECK here.
   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.
   size_t uv_offset = (video_frame->stride(media::VideoFrame::kUPlane) *
                       (frame_clip_top >> y_shift)) + (frame_clip_left >> 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;
 
-  // 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,
                          frame_clip_u,
                          frame_clip_v,
                          dest_rect_pointer,
                          frame_clip_width,
                          frame_clip_height,
                          local_dest_irect.width(),
                          local_dest_irect.height(),
@@ skipping 14 matching lines @@
     SkBitmap* bitmap) {
   DCHECK(IsEitherYV12OrYV16OrNative(video_frame->format()))
       << video_frame->format();
   if (IsEitherYV12OrYV16(video_frame->format())) {
     DCHECK_EQ(video_frame->stride(media::VideoFrame::kUPlane),
               video_frame->stride(media::VideoFrame::kVPlane));
   }
 
   // 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(),
                              yuv_type);
   } else {
     DCHECK_EQ(video_frame->format(), media::VideoFrame::NATIVE_TEXTURE);
     video_frame->ReadPixelsFromNativeTexture(bitmap->getPixels());
   }
   bitmap->notifyPixelsChanged();
   bitmap->unlockPixels();
@@ skipping 38 matching lines @@
     ConvertVideoFrameToBitmap(video_frame, &last_frame_);
     last_frame_timestamp_ = video_frame->GetTimestamp();
   }
 
   // Do a slower paint using |last_frame_|.
   paint.setFilterBitmap(true);
   canvas->drawBitmapRect(last_frame_, NULL, dest, &paint);
 }
 
 }  // namespace media