Consolidate Windows bitmap and DC code

skia/ext/skia_utils_win.cc

Index: skia/ext/skia_utils_win.cc
diff --git a/skia/ext/skia_utils_win.cc b/skia/ext/skia_utils_win.cc
index 9873373725705fe6f8e4491a54ebd56916829276..3db97dcd18b81754afe96939f8775e986a4123ac 100644
--- a/skia/ext/skia_utils_win.cc
+++ b/skia/ext/skia_utils_win.cc
@@ -7,7 +7,10 @@
 #include <stddef.h>
 #include <windows.h>
 
+#include "base/debug/gdi_debug_util_win.h"
+#include "base/win/win_util.h"
 #include "third_party/skia/include/core/SkRect.h"
+#include "third_party/skia/include/core/SkSurface.h"
 #include "third_party/skia/include/core/SkTypes.h"
 #include "third_party/skia/include/effects/SkGradientShader.h"
 
@@ -23,8 +26,42 @@ static_assert(sizeof(RECT().right) == sizeof(SkIRect().fRight), "o7");
 static_assert(sizeof(RECT().bottom) == sizeof(SkIRect().fBottom), "o8");
 static_assert(sizeof(RECT) == sizeof(SkIRect), "o9");
 
-}  // namespace
+HBITMAP CreateHBitmap(int width, int height) {
+  // CreateDIBSection appears to get unhappy if we create an empty bitmap, so

[Peter Kasting, 2016/02/11 00:19:25]

What does "appears to get unhappy" mean?  Be specific rather than colloquial.

[tomhudson, 2016/02/16 17:47:54]

This comment comes verbatim out of skia/ext/bitmap_platform_device_win.cc, which
we're trying to get rid of (and so the code being copied now will be deleted in
the future). Agree that it's not clear; it dates back to at least 2008, so I
suspect nobody knows specifically what it means.

[Peter Kasting, 2016/02/16 21:12:09]

Can you try a quick runtime test to see what happens when creating an empty
bitmap, so this can be updated?  I'd rather not just preserve comments that no
one understands, but this does deserve a comment, so we should really understand
why it's necessary.

[tomhudson, 2016/06/21 22:20:22]

Under 64b Windows 7, if I write
  HBITMAP hb = CreateHBitmap(0, 0)
that triggers the CHECK(false) in line 59 of gdi_debug_util_win.cc; I suspect
that the CreateDIBSection() call is failing to allocate and our crash-recovery
code is also failing.

I could change the comment to be explicit about the failure mode?

Note also that we do the same thing on Mac (make sure generated bitmaps are at
least 1x1).

So - contrary to Carlos' expectations around the old #if !defined(_WIN64), it
looks like we can trigger GDI failure on 64b?

[Peter Kasting, 2016/06/22 02:23:33]

Yes, please update the comment to be clearer about this.
Not sure whether this kind of case is what he meant when he said to ping him if
this can fail.

+  // just create a minimal bitmap

[Peter Kasting, 2016/02/11 00:19:26]

Nit: Trailing period

[tomhudson, 2016/02/16 17:47:54]

Done.

+  if ((width == 0) || (height == 0)) {
+    width = 1;
+    height = 1;
+  }

[Peter Kasting, 2016/02/11 00:19:25]

Nit: Do we really want to force e.g. (30, 0) to (1, 1) instead of (30, 1)?  If
not, then this is simpler:

  width = std::max(width, 1);
  height = std::max(height, 1);

[tomhudson, 2016/02/16 17:47:54]

https://codereview.chromium.org/9459 changed it from (30, 1) to (1, 1), reading
the bug history behind it that might have been preventative rather than
addressing an observed bug?

Skia will treat a bitmap with 0 in width *or* height as empty, so I suspect we
want to do the minimal thing to make the OS happy, and a 1 pixel bitmap is
smaller than a 30 pixel bitmap.

+
+  BITMAPINFOHEADER hdr = {0};
+  hdr.biSize = sizeof(BITMAPINFOHEADER);
+  hdr.biWidth = width;
+  hdr.biHeight = -height;  // minus means top-down bitmap
+  hdr.biPlanes = 1;
+  hdr.biBitCount = 32;
+  hdr.biCompression = BI_RGB;  // no compression
+  hdr.biSizeImage = 0;
+  hdr.biXPelsPerMeter = 1;
+  hdr.biYPelsPerMeter = 1;
+  hdr.biClrUsed = 0;
+  hdr.biClrImportant = 0;

[Peter Kasting, 2016/02/11 00:19:26]

Don't do all this manually; use gfx::CreateBitmapHeader().

[tomhudson, 2016/02/16 17:47:54]

Dependency is in the wrong direction? gfx depends on Skia; I don't want to make
Skia depend on gfx. (One of the questions we want to investigate this year is:
since we only have a single backend for Chrome (Skia), what parts of gfx are now
unnecessary?)

I'd be happy to consider moving that function here; it seems to be used in 3
places:

* printing/emf_win: might go away now that XP support is ended, but there aren't
people dedicated to that yet; is already including this file.
* ui/base/dragdrop: caller "CreateHBITMAPFromSkBitmap()" really ought to be in
skia/ext, if we don't have an equivalent function already.
* ui/gfx/icon_util: is already using skia/ext/image_operations.

[Peter Kasting, 2016/02/16 21:12:09]

Sounds like we should go ahead and move that function here.  I would do that in
this patch; moving CreateHBITMAPFromSkBitmap() can be done separately if you
want to do it.

[tomhudson, 2016/06/21 22:20:22]

Done.

+
+  HBITMAP hbitmap = CreateDIBSection(NULL, reinterpret_cast<BITMAPINFO*>(&hdr),
+                                     0, NULL, NULL, 0);
+
+#if !defined(_WIN64)

[Peter Kasting, 2016/02/11 00:19:25]

Why is this ifdefed?

[tomhudson, 2016/02/16 17:47:54]

cpu@ added that to try to isolate reasons for beta crashes two years ago today
(https://codereview.chromium.org/159523003); there's no discussion of why in the
code review or in the bug so far as I can see.

[Peter Kasting, 2016/02/16 21:12:09]

Can you ping Carlos and try to find out why this is ifdefed?

[cpu_(ooo_6.6-7.5), 2016/06/12 19:13:16]

Looking at the code I use windows's toohelp which might have word-size dependent
types, at that point the x64 build was probably a baby and I didn't feel like
spending time on that. I think it is fine to enable in 64 bits.

I will be quite surprising if this fires in 64bits. If so please ping me. I can
page back into my brain all that suff if needed.

+  // If this call fails, we're gonna crash hard. Try to get some useful

[Peter Kasting, 2016/02/11 00:19:26]

Which call?  The call above?  And why/where will we crash?

Nit: Again, avoid colloquialisms like "gonna crash hard".

[tomhudson, 2016/02/16 17:47:54]

Acknowledged.

Another legacy comment. Changing to
  // If CreateDIBSection() failed, try to get some useful
  // information out before we crash for post-mortem analysis.

+  // information out before we crash for post-mortem analysis.
+  if (!hbitmap)
+    base::debug::GDIBitmapAllocFailure(&hdr, NULL);
+#endif
+
+  return hbitmap;
+}
+
 
+}  // namespace
 namespace skia {
 
 POINT SkPointToPOINT(const SkPoint& point) {
@@ -97,5 +134,61 @@ void InitializeDC(HDC context) {
   SkASSERT(res != 0);
 }
 
+SkSurface* MapPlatformSurface(HDC context) {
+  HBITMAP backing_handle = (HBITMAP)GetCurrentObject(context, OBJ_BITMAP);

[Peter Kasting, 2016/02/11 00:19:25]

No C-style casts (2 places)

[tomhudson, 2016/02/16 17:47:53]

Done.

+  BITMAP backing;
+  int byte_count = GetObject(backing_handle, sizeof(backing),
+                             &backing);
+  if (byte_count != sizeof(backing))

[Peter Kasting, 2016/02/11 00:19:25]

Nit: Just inline this test above to remove the temp (2 places)

[tomhudson, 2016/02/16 17:47:54]

Done.

* Is there something I'm missing in the style guide, or is this just a personal
preference?

+    return nullptr;
+  SkImageInfo size = SkImageInfo::MakeN32Premul(backing.bmWidth,
+                                                backing.bmHeight);
+  return SkSurface::NewRasterDirect(size, backing.bmBits,
+                                    backing.bmWidthBytes);

[Peter Kasting, 2016/02/11 00:19:25]

Nit: Shorter:

  return SkSurface::NewRasterDirect(
      SkImageInfo::MakeN32Premul(backing.bmWidth, backing.bmHeight),
      backing.bmBits, backing.bmWidthBytes);

[tomhudson, 2016/02/16 17:47:54]

Done.

+}
+
+SkBitmap MapPlatformBitmap(HDC context) {
+  HBITMAP backing_handle = (HBITMAP)GetCurrentObject(context, OBJ_BITMAP);
+  BITMAP backing;
+  SkBitmap bitmap;
+  int byte_count = GetObject(backing_handle, sizeof(backing),
+                             &backing);
+  if (byte_count != sizeof(backing))
+    return bitmap;
+  SkImageInfo size = SkImageInfo::MakeN32Premul(backing.bmWidth,
+                                                backing.bmHeight);
+  bitmap.installPixels(size, backing.bmBits, size.minRowBytes());

[Peter Kasting, 2016/02/11 00:19:26]

Everything about this function above this line is identical to the function
above.  Use a helper function to avoid code duplication.

[tomhudson, 2016/02/16 17:47:54]

I'm not convinced that a helper function is right here. When we have three
function calls, an early out, and multiple things that would need to be returned
from the helper function, the interface gets pretty ugly and the code gets ~30%
longer. Or is there a better way to write a helper function than this?

namespace {

bool PrepareAllocation(HDC context, SkImageInfo* size, BITMAP* backing) {
  HBITMAP backing_handle = 
    static_cast<HBITMAP>GetCurrentObject(context, OBJ_BITMAP);
  if (GetObject(backing_handle, sizeof(*backing),
                             backing) != sizeof(*backing))
    return false;
  *size = SkImageInfo::MakeN32Premul(backing->bmWidth,
                                     backing->bmHeight);
  return true;
}

}

SkSurface* MapPlatformSurface(HDC context) {
  SkImageInfo size;
  Bitmap backing;
  if (!PrepareAllocation(HDC, &size, &backing))
    return nullptr;
  return SkSurface::NewRasterDirect(size, backing.bmBits,
                                    backing.bmWidthBytes);
}

SkBitmap MapPlatformBitmap(HDC context) {
  SkImageInfo size;
  Bitmap backing;
  SkBitmap bitmap;
  if (PrepareAllocation(HDC, &size, &backing))
    bitmap.installPixels(size, backing.bmBits, size.minRowBytes());
  return bitmap;
}

[Peter Kasting, 2016/02/16 21:12:09]

Assuming you don't need to be able to allocate empty-sized objects, you can just
return the size, which cleans things up a bit:

SkImageInfo PrepareAllocation(HDC context, BITMAP* backing) {
  HBITMAP backing_handle =
      static_cast<HBITMAP>GetCurrentObject(context, OBJ_BITMAP);
  const size_t backing_size = sizeof *backing;
  return (GetObject(backing_handle, backing_size, backing) == backing_size)
      ? SkImageInfo::MakeN32Premul(backing->bmWidth, backing->bmHeight)
      : SkImageInfo();
}

SkSurface* MapPlatformSurface(HDC context) {
  Bitmap backing;
  const SkImageInfo size(PrepareAllocation(HDC, &backing));
  return size.isEmpty()
      ? nullptr
      : SkSurface::NewRasterDirect(size, backing.bmBits, backing.bmWidthBytes);
}

SkBitmap MapPlatformBitmap(HDC context) {
  Bitmap backing;
  const SkImageInfo size(PrepareAllocation(HDC, &backing));
  SkBitmap bitmap;
  if (!size.isEmpty())
    bitmap.installPixels(size, backing.bmBits, size.minRowBytes());
  return bitmap;
}

This is still a bit longer than the original, but seems a bit clearer/safer.

[tomhudson, 2016/06/21 22:20:22]

...
Done. Thanks for writing the improved version.

+  return bitmap;
+}
+
+HDC CreateOffscreenSurface(int width, int height) {
+

[Peter Kasting, 2016/02/11 00:19:26]

Nit: Remove this blank line

[tomhudson, 2016/02/16 17:47:54]

Done.

+  HBITMAP bitmap = nullptr;

[Peter Kasting, 2016/02/11 00:19:26]

Your code seems to mix NULL and nullptr without apparent rationale.  Be
consistent (and prefer to consistently use nullptr, if possible).

[tomhudson, 2016/02/16 17:47:54]

Done.

+
+  // If this process doesn't have access to GDI, we'll have to use a shared
+  // memory segment instead.
+  if (!base::win::IsUser32AndGdi32Available())
+    return NULL;
+
+  bitmap = CreateHBitmap(width, height);
+  if (!bitmap)
+    return NULL;
+
+  HDC hdc = CreateCompatibleDC(NULL);
+  SkASSERT(hdc);
+  InitializeDC(hdc);
+  HRGN clip = CreateRectRgn(0, 0, width, height);
+  int result = SelectClipRgn(hdc, clip);
+  SkASSERT(result != ERROR);
+  result = DeleteObject(clip);
+  SkASSERT(result);
+
+  SelectObject(hdc, bitmap);
+
+  // need to clean up by calling DeleteDC(hdc) else we leak!

[Peter Kasting, 2016/02/11 00:19:26]

So why aren't we doing that?

[tomhudson, 2016/02/16 17:47:54]

Returning HDC is the entire point of this function. More properly, the *caller*
must DeleteDC(hdc); that's mentioned in the comments in the header file.

[Peter Kasting, 2016/02/16 21:12:09]

In that case, I would change this to:

  // The caller must DeleteDC(hdc) on this object once it's done.
  return hdc;

[tomhudson, 2016/06/21 22:20:22]

Done.

+
+  return hdc;
+}
+
 }  // namespace skia