stop using copyPixelsTo -- deprecated

remoting/test/frame_generator_util.cc

 // Copyright 2016 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 "remoting/test/frame_generator_util.h"
 
 #include "base/base_paths.h"
 #include "base/files/file_util.h"
 #include "base/path_service.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 #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 {
 
 std::unique_ptr<webrtc::DesktopFrame> LoadDesktopFrameFromPng(
     const char* name) {
   base::FilePath file_path;
   PathService::Get(base::DIR_SOURCE_ROOT, &file_path);
   file_path = file_path.AppendASCII("remoting");
   file_path = file_path.AppendASCII("test");
   file_path = file_path.AppendASCII("data");
   file_path = file_path.AppendASCII(name);
 
   std::string file_content;
   if (!base::ReadFileToString(file_path, &file_content))
     LOG(FATAL) << "Failed to read " << file_path.MaybeAsASCII()
                << ". Please run remoting/test/data/download.sh";
   SkBitmap bitmap;
   gfx::PNGCodec::Decode(reinterpret_cast<const uint8_t*>(file_content.data()),
                         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;
 }
 
 void DrawRect(webrtc::DesktopFrame* frame,
               webrtc::DesktopRect rect,
               uint32_t color) {
   for (int y = rect.top(); y < rect.bottom(); ++y) {
     uint32_t* data = reinterpret_cast<uint32_t*>(
         frame->GetFrameDataAtPos(webrtc::DesktopVector(rect.left(), y)));
     for (int x = 0; x < rect.width(); ++x) {
       data[x] = color;
     }
   }
 }
 
 }  // namespace test
 }  // namespace remoting