Introducing a helper wrapper for YUV-to-RGB convertion and/or scaling.

remoting/base/util.cc

Index: remoting/base/util.cc
diff --git a/remoting/base/util.cc b/remoting/base/util.cc
index 486b6a3510e6f42efa1680d449d7363628aa4b14..8889c6430bd7a90b93970b58db5847fc0d20cb62 100644
--- a/remoting/base/util.cc
+++ b/remoting/base/util.cc
@@ -48,38 +48,15 @@ static int CalculateRGBOffset(int x, int y, int stride) {
 }
 
 static int CalculateYOffset(int x, int y, int stride) {
+  DCHECK(((x & 1) == 0) && ((y & 1) == 0));
   return stride * y + x;
 }
 
 static int CalculateUVOffset(int x, int y, int stride) {
+  DCHECK(((x & 1) == 0) && ((y & 1) == 0));
   return stride * y / 2 + x / 2;
 }
 
-void ConvertYUVToRGB32WithRect(const uint8* y_plane,
-                               const uint8* u_plane,
-                               const uint8* v_plane,
-                               uint8* rgb_plane,
-                               const SkIRect& rect,
-                               int y_stride,
-                               int uv_stride,
-                               int rgb_stride) {
-  DCHECK((rect.x() & 1) == 0 && (rect.y() & 1) == 0);
-  int rgb_offset = CalculateRGBOffset(rect.left(), rect.top(), rgb_stride);
-  int y_offset = CalculateYOffset(rect.left(), rect.top(), y_stride);
-  int uv_offset = CalculateUVOffset(rect.left(), rect.top(), uv_stride);
-
-  media::ConvertYUVToRGB32(y_plane + y_offset,
-                           u_plane + uv_offset,
-                           v_plane + uv_offset,
-                           rgb_plane + rgb_offset,
-                           rect.width(),
-                           rect.height(),
-                           y_stride,
-                           uv_stride,
-                           rgb_stride,
-                           media::YV12);
-}
-
 void ConvertRGB32ToYUVWithRect(const uint8* rgb_plane,
                                uint8* y_plane,
                                uint8* u_plane,
@@ -106,6 +83,176 @@ void ConvertRGB32ToYUVWithRect(const uint8* rgb_plane,
                            uv_stride);
 }
 
+void ConvertAndScaleYUVToRGB32Rect(const uint8* source_yplane,
+                                   const uint8* source_uplane,
+                                   const uint8* source_vplane,
+                                   int source_ystride,
+                                   int source_uvstride,
+                                   const SkISize& source_size,
+                                   const SkIRect& source_buffer_rect,
+                                   uint8* dest_buffer,
+                                   int dest_stride,
+                                   const SkISize& dest_size,
+                                   const SkIRect& dest_buffer_rect,
+                                   const SkIRect& dest_rect) {
+  // N.B. It is caller's responsibility to check if strides are large enough. We
+  // cannot do it here anyway.
+  DCHECK(SkIRect::MakeSize(source_size).contains(source_buffer_rect));
+  DCHECK(SkIRect::MakeSize(dest_size).contains(dest_buffer_rect));
+  DCHECK(dest_buffer_rect.contains(dest_rect));
+  DCHECK(ScaleRect(source_buffer_rect, source_size, dest_size).
+             contains(dest_rect));
+
+  // The top left corner coordinates of YUV buffer should have even X and Y
+  // coordinates.
+  DCHECK(source_buffer_rect == AlignRect(source_buffer_rect));

[Wez, 2012/02/03 22:40:35]

Don't CalculateYOffset() and CalculateUVOffset() already verify the alignment?

[alexeypa (please no reviews), 2012/02/03 23:29:35]

Done.

+
+  // If the source and/or destination buffers don't start at (0, 0)
+  // offset the pointers to pretend we have complete buffers.
+  int y_offset = - CalculateYOffset(source_buffer_rect.x(),
+                                    source_buffer_rect.y(),
+                                    source_ystride);
+  int uv_offset = - CalculateUVOffset(source_buffer_rect.x(),
+                                      source_buffer_rect.y(),
+                                      source_uvstride);
+  int rgb_offset = - CalculateRGBOffset(dest_buffer_rect.x(),
+                                        dest_buffer_rect.y(),
+                                        dest_stride);
+
+  // See if scaling is needed.
+  if (source_size == dest_size) {
+    // Calculate the inner rectangle that can be copied by the optimized
+    // ConvertYUVToRGB32().

[Wez, 2012/02/03 22:40:35]

Why are we calculating a smaller aligned rect than the destination?

[alexeypa (please no reviews), 2012/02/03 23:29:35]

We are not allowed to touch pixels outside of the region. That would be really
confusing for the caller. So the only way to copy an aligned rect by the
optimized routine is to copy a smaller one.

On 2012/02/03 22:40:35, Wez wrote:

[Wez, 2012/02/04 01:03:43]

I think it's reasonable to require the caller to arrange alignment correctly. 
In the non-scaling case it involves having a buffer that is at most 1 pixel
wider and higher than the clip area, and it greatly simplifies this code.

[alexeypa (please no reviews), 2012/02/06 05:45:16]

I'll stick to this approach for now. However I'll re-evaluate the alignment
requirements when we will be switching to libyuv.

On 2012/02/04 01:03:43, Wez wrote:

+    SkIRect inner_rect =
+        SkIRect::MakeLTRB(RoundToTwosMultiple(dest_rect.left() + 1),
+                          RoundToTwosMultiple(dest_rect.top() + 1),
+                          RoundToTwosMultiple(dest_rect.right()),
+                          RoundToTwosMultiple(dest_rect.bottom()));
+
+    // Offset pointers to point to the top left corner of the inner rectangle.
+    y_offset += CalculateYOffset(inner_rect.x(), inner_rect.y(),
+                                 source_ystride);
+    uv_offset += CalculateUVOffset(inner_rect.x(), inner_rect.y(),
+                                   source_uvstride);
+    rgb_offset += CalculateRGBOffset(inner_rect.x(), inner_rect.y(),
+                                     dest_stride);
+
+    media::ConvertYUVToRGB32(source_yplane + y_offset,
+                             source_uplane + uv_offset,
+                             source_vplane + uv_offset,
+                             dest_buffer + rgb_offset,
+                             inner_rect.width(),
+                             inner_rect.height(),
+                             source_ystride,
+                             source_uvstride,
+                             dest_stride,
+                             media::YV12);
+
+    // Now see if some pixels weren't copied due to alignment.

[Wez, 2012/02/03 22:40:35]

Why not just align the top and left of the rectangle to even coordinates and
avoid all this complexity?

[alexeypa (please no reviews), 2012/02/03 23:29:35]

There are a few reasons I settled on this approach.

Alignment requirement is only a consequence of using ConvertYUVToRGB32() which
itself is used as a performance optimization only. Enforcing alignment when
scaling is performed would be very confusing for the caller because it does not
make any sense - the accesses to YUV buffer would be not aligned anyway.

On the other hand removing the alignment requirement reduces  caller's head ace
a lot. Now there is not need to track down 
 all the paths were rectangles are being changed. Performance-sensitive callers
still can align buffers to avoid any additional cost.

And at last doing it this way preserves the optimization because the bulk of
pixels is still copied by an optimized routine. (Which makes me think that the
condition choosing 1:1 or scaling should also see how large is the region that
we are coping and use scaling function to deal with small unaligned rectangles.


On 2012/02/03 22:40:35, Wez wrote:

[Wez, 2012/02/04 01:03:43]

Your point is that in the 1:1 case the caller needs to worry about alignment,
but not otherwise?  Fair point.

If you want to stick with this approach then you could simplify this code by
something like:

  SkRegion edges(original_rect);
  edges.op(aligned_inner_rect, kSubtract);
  for (SkIRect in edges)
    ScaleYUVToRGB32WithRect(...)

[alexeypa (please no reviews), 2012/02/06 05:45:16]

Done.

+    SkIRect outer_rect = AlignRect(dest_rect);
+    SkIPoint offset = SkIPoint::Make(outer_rect.x() - inner_rect.x(),
+                                     outer_rect.y() - inner_rect.y());
+
+    // Offset the pointers to point to the top left corner of the outer
+    // rectangle.
+    y_offset += CalculateYOffset(offset.x(), offset.y(), source_ystride);
+    uv_offset += CalculateUVOffset(offset.x(), offset.y(), source_uvstride);
+    rgb_offset += CalculateRGBOffset(offset.x(), offset.y(), dest_stride);
+
+    // Draw the top line.
+    if (inner_rect.top() != dest_rect.top()) {
+      media::ScaleYUVToRGB32WithRect(source_yplane + y_offset,
+                                     source_uplane + uv_offset,
+                                     source_vplane + uv_offset,
+                                     dest_buffer + rgb_offset,
+                                     source_size.width(),
+                                     source_size.height(),
+                                     dest_size.width(),
+                                     dest_size.height(),
+                                     dest_rect.left() - outer_rect.left(),
+                                     dest_rect.top() - outer_rect.top(),
+                                     dest_rect.right() - outer_rect.left(),
+                                     inner_rect.top() - outer_rect.top(),
+                                     source_ystride,
+                                     source_uvstride,
+                                     dest_stride);
+    }
+
+    // Draw the bottom line.
+    if (inner_rect.bottom() != dest_rect.bottom()) {
+      media::ScaleYUVToRGB32WithRect(source_yplane + y_offset,
+                                     source_uplane + uv_offset,
+                                     source_vplane + uv_offset,
+                                     dest_buffer + rgb_offset,
+                                     source_size.width(),
+                                     source_size.height(),
+                                     dest_size.width(),
+                                     dest_size.height(),
+                                     dest_rect.left() - outer_rect.left(),
+                                     inner_rect.bottom() - outer_rect.top(),
+                                     dest_rect.right() - outer_rect.left(),
+                                     dest_rect.bottom() - outer_rect.top(),
+                                     source_ystride,
+                                     source_uvstride,
+                                     dest_stride);
+    }
+
+    // Draw the left column.
+    if (inner_rect.left() != dest_rect.left()) {
+      media::ScaleYUVToRGB32WithRect(source_yplane + y_offset,
+                                     source_uplane + uv_offset,
+                                     source_vplane + uv_offset,
+                                     dest_buffer + rgb_offset,
+                                     source_size.width(),
+                                     source_size.height(),
+                                     dest_size.width(),
+                                     dest_size.height(),
+                                     dest_rect.left() - outer_rect.left(),
+                                     dest_rect.top() - outer_rect.top(),
+                                     inner_rect.left() - outer_rect.left(),
+                                     dest_rect.bottom() - outer_rect.top(),
+                                     source_ystride,
+                                     source_uvstride,
+                                     dest_stride);
+    }
+
+    // Draw the right column.
+    if (inner_rect.right() != dest_rect.right()) {
+      media::ScaleYUVToRGB32WithRect(source_yplane + y_offset,
+                                     source_uplane + uv_offset,
+                                     source_vplane + uv_offset,
+                                     dest_buffer + rgb_offset,
+                                     source_size.width(),
+                                     source_size.height(),
+                                     dest_size.width(),
+                                     dest_size.height(),
+                                     inner_rect.right() - outer_rect.left(),
+                                     dest_rect.top() - outer_rect.top(),
+                                     dest_rect.right() - outer_rect.left(),
+                                     dest_rect.bottom() - outer_rect.top(),
+                                     source_ystride,
+                                     source_uvstride,
+                                     dest_stride);
+    }
+  } else {
+    media::ScaleYUVToRGB32WithRect(source_yplane + y_offset,
+                                   source_uplane + uv_offset,
+                                   source_vplane + uv_offset,
+                                   dest_buffer + rgb_offset,
+                                   source_size.width(),
+                                   source_size.height(),
+                                   dest_size.width(),
+                                   dest_size.height(),
+                                   dest_rect.left(),
+                                   dest_rect.top(),
+                                   dest_rect.right(),
+                                   dest_rect.bottom(),
+                                   source_ystride,
+                                   source_uvstride,
+                                   dest_stride);
+  }
+}
+
 int RoundToTwosMultiple(int x) {
   return x & (~1);
 }
@@ -153,4 +300,31 @@ void CopyRect(const uint8* src_plane,
   }
 }
 
+void CopyRGB32Rect(const uint8* source_buffer,
+                   int source_stride,
+                   const SkIRect& source_buffer_rect,
+                   uint8* dest_buffer,
+                   int dest_stride,
+                   const SkIRect& dest_buffer_rect,
+                   const SkIRect& dest_rect) {
+  DCHECK(dest_buffer_rect.contains(dest_rect));
+  DCHECK(source_buffer_rect.contains(dest_rect));
+
+  // Get the address of the starting point.
+  int source_offset = CalculateRGBOffset(dest_rect.x() - source_buffer_rect.x(),
+                                         dest_rect.y() - source_buffer_rect.y(),
+                                         source_stride);
+  int dest_offset = CalculateRGBOffset(dest_rect.x() - dest_buffer_rect.x(),
+                                       dest_rect.y() - dest_buffer_rect.y(),
+                                       source_stride);
+
+  // Copy bits.
+  CopyRect(source_buffer + source_offset,
+           source_stride,
+           dest_buffer + dest_offset,
+           dest_stride,
+           GetBytesPerPixel(media::VideoFrame::RGB32),
+           SkIRect::MakeWH(dest_rect.width(), dest_rect.height()));

[Wez, 2012/02/03 22:40:35]

Is there no SkRect::size() method?

[alexeypa (please no reviews), 2012/02/03 23:29:35]

Nope. SkIRect.offset does not work as well because it would modify the object in
place.

On 2012/02/03 22:40:35, Wez wrote:

+}
+
 }  // namespace remoting