stop using copyPixelsTo -- deprecated

remoting/test/frame_generator_util.cc

Index: remoting/test/frame_generator_util.cc
diff --git a/remoting/test/frame_generator_util.cc b/remoting/test/frame_generator_util.cc
index bf78b0f14ae415af1c2a53d9d4c7a46d4efded32..bb52daeff68772891e8e35f7dc7ff2acc01e3d1e 100644
--- a/remoting/test/frame_generator_util.cc
+++ b/remoting/test/frame_generator_util.cc
@@ -12,6 +12,25 @@
 #include "third_party/webrtc/modules/desktop_capture/desktop_geometry.h"
 #include "ui/gfx/codec/png_codec.h"
 
+namespace {
+void CopyPixelsToBuffer(const SkBitmap& src,
+                        void* dst_pixels,
+                        size_t dst_stride) {

[Sergey Ulanov, 2017/04/06 19:04:28]

In webrtc::DesktopFrame stride may be negative, so this should be int. This
doesn't matter here because in BasicDesktopFrame stride is always positive,
still better to use int.

[reed1, 2017/04/07 12:28:13]

The old logic cast it to size_t when it called copyPixelsTo(). What is the
meaning of negative? Should this function fail if it sees a negative frame
stride?

[Sergey Ulanov, 2017/04/07 18:05:12]

It's used for the case when rows are stored from bottom to top. This is useful
for some capturers, see
https://codesearch.chromium.org/chromium/src/third_party/webrtc/modules/deskt...

In all cases DesktopFrame::data() points to the top row and adding stride()
moves to the row below it.
No, it will still work correctly with negative stride as is. Just need to remove
the DCHECK and use signed type for dst_stride.

[reed1, 2017/04/07 21:11:25]

Got it. The old code-path (the method being deprecated) was definitely not ready
for a negative stride.

+  SkBitmap tmp(src);
+  tmp.lockPixels();
+  const char* src_pixels = static_cast<const char*>(tmp.getPixels());
+  size_t src_stride = tmp.rowBytes();
+  // only need to copy the important parts of the row

[Sergey Ulanov, 2017/04/06 19:04:28]

I don't think this comment is necessary, but if you keep it then please change
it to "Only need to copy the important parts of the row." (capital O, period at
the end).

[reed1, 2017/04/07 12:28:13]

Done.

+  size_t bytes_per_row = tmp.width() * tmp.bytesPerPixel();
+  DCHECK(bytes_per_row <= dst_stride);

[Sergey Ulanov, 2017/04/06 19:04:28]

stride may be negative for webrtc::DesktopFrame, so I don't think this DCHECK is
valid, just remove it?

[reed1, 2017/04/07 12:28:13]

Should this fail if it sees a negative?

[Sergey Ulanov, 2017/04/07 18:05:12]

No.

[reed1, 2017/04/07 21:11:25]

Done.

+  for (int y = 0; y < tmp.height(); ++y) {
+    memcpy(dst_pixels, src_pixels, bytes_per_row);
+    src_pixels += src_stride;
+    dst_pixels = (char*)dst_pixels + dst_stride;

[Sergey Ulanov, 2017/04/06 19:04:28]

I don't think you need the cast here. 
  dst_pixels += dst_stride;
This is more consistent with the previous line.
In either case it shouldn't be C-style cast:
https://google.github.io/styleguide/cppguide.html#Casting

[reed1, 2017/04/07 12:28:13]

dst_pixels is void*, so it cannot perform += without a cast.

Will use c++ style cast.

[Sergey Ulanov, 2017/04/07 18:05:12]

Maybe change it to uint8_t*? It's the type used for DesktopFrame::data() and I
don't think it's necessary to cast it to void* when CopyPixelsToBuffer() is
called.

[reed1, 2017/04/07 21:11:25]

Done.

+  }
+}
+}

[Sergey Ulanov, 2017/04/06 19:04:28]

 // namespace 

[reed1, 2017/04/07 12:28:13]

Done.

+
 namespace remoting {
 namespace test {
 
@@ -33,9 +52,7 @@ std::unique_ptr<webrtc::DesktopFrame> LoadDesktopFrameFromPng(
                         file_content.size(), &bitmap);
   std::unique_ptr<webrtc::DesktopFrame> frame(new webrtc::BasicDesktopFrame(
       webrtc::DesktopSize(bitmap.width(), bitmap.height())));
-  bitmap.copyPixelsTo(frame->data(),
-                      frame->stride() * frame->size().height(),
-                      frame->stride());
+  CopyPixelsToBuffer(bitmap, frame->data(), frame->stride());
   return frame;
 }