163668 2016-10-19 10:11:04 -0700 [Win][Direct2D] Implement ImageBufferData::getData. 2016-10-20 02:09:14 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 pvollan pvollan achristensen bfulgham commit-queue oldest_to_newest 1241958 0 pvollan 2016-10-19 10:11:04 -0700 This method is not yet implemented for Direct2D. 1241960 1 292078 pvollan 2016-10-19 10:14:20 -0700 Created attachment 292078 Patch 1241961 2 commit-queue 2016-10-19 10:15:21 -0700 Attachment 292078 did not pass style-queue: ERROR: Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:78: Use the class HWndDC instead of calling GetDC to avoid potential memory leaks. [runtime/leaky_pattern] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style. 1241979 3 292078 bfulgham 2016-10-19 10:38:32 -0700 Comment on attachment 292078 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292078&action=review Thanks for getting this to work! I had a few minor comments I'd like you to address when landing. > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:50 > + area *= rect.height(); You should use the "area()" method: area = 4 * rect.area(); > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:71 > + hr = platformContext->QueryInterface(__uuidof(ID2D1GdiInteropRenderTarget), (void**)&gdiRenderTarget); I think we should retain this in the GraphicsContextPlatformPrivate, since there will be other times we want to get a GDI HBITMAP or similar out of our D2D context. You can do this as a separate patch. > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:89 > + memcpy(result->data(), pixels, 4 * rect.width() * rect.height()); Can we avoid this extra copy by somehow using result->data() as the pixel source? 1242296 4 pvollan 2016-10-20 01:50:04 -0700 (In reply to comment #3) > Comment on attachment 292078 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=292078&action=review > > Thanks for getting this to work! I had a few minor comments I'd like you to > address when landing. > > > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:50 > > + area *= rect.height(); > > You should use the "area()" method: area = 4 * rect.area(); > > > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:71 > > + hr = platformContext->QueryInterface(__uuidof(ID2D1GdiInteropRenderTarget), (void**)&gdiRenderTarget); > > I think we should retain this in the GraphicsContextPlatformPrivate, since > there will be other times we want to get a GDI HBITMAP or similar out of our > D2D context. > > You can do this as a separate patch. > > > Source/WebCore/platform/graphics/win/ImageBufferDataDirect2D.cpp:89 > > + memcpy(result->data(), pixels, 4 * rect.width() * rect.height()); > > Can we avoid this extra copy by somehow using result->data() as the pixel > source? I haven't found a way to do this yet, since the system is allocating the bitmap pixel data for us. Thanks for reviewing! 1242301 5 pvollan 2016-10-20 02:09:14 -0700 Committed r207591: <http://trac.webkit.org/changeset/207591> 292078 2016-10-19 10:14:20 -0700 2016-10-19 10:38:32 -0700 Patch bug-163668-20161019101043.patch text/plain 4424 pvollan SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwNzU0MSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDE2LTEwLTE5ICBQZXIgQXJu ZSBWb2xsYW4gIDxwdm9sbGFuQGFwcGxlLmNvbT4KKworICAgICAgICBbV2luXVtEaXJlY3QyRF0g SW1wbGVtZW50IEltYWdlQnVmZmVyRGF0YTo6Z2V0RGF0YS4KKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE2MzY2OAorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFJlbmRlciBkYXRhIHRvIGEgYml0bWFwIGluIHN5 c3RlbSBtZW1vcnksIHdoaWNoIGRhdGEgY2FuIGJlIHJlYWQgZnJvbS4KKworICAgICAgICAqIHBs YXRmb3JtL2dyYXBoaWNzL3dpbi9JbWFnZUJ1ZmZlckRhdGFEaXJlY3QyRC5jcHA6CisgICAgICAg IChXZWJDb3JlOjpJbWFnZUJ1ZmZlckRhdGE6OmdldERhdGEpOgorICAgICAgICAqIHBsYXRmb3Jt L2dyYXBoaWNzL3dpbi9JbWFnZUJ1ZmZlckRpcmVjdDJELmNwcDoKKyAgICAgICAgKFdlYkNvcmU6 OkltYWdlQnVmZmVyOjpJbWFnZUJ1ZmZlcik6CisKIDIwMTYtMTAtMTkgIERhcmluIEFkbGVyICA8 ZGFyaW5AYXBwbGUuY29tPgogCiAgICAgICAgIE1vdmUgWFBhdGggZnJvbSBFeGNlcHRpb25Db2Rl IHRvIEV4Y2VwdGlvbgpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvd2lu L0ltYWdlQnVmZmVyRGF0YURpcmVjdDJELmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29y ZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4vSW1hZ2VCdWZmZXJEYXRhRGlyZWN0MkQuY3BwCShyZXZp c2lvbiAyMDc1MjYpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4vSW1h Z2VCdWZmZXJEYXRhRGlyZWN0MkQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yOCw3ICsyOCw5IEBA CiAKICNpZiBVU0UoRElSRUNUMkQpCiAKKyNpbmNsdWRlICJCaXRtYXBJbmZvLmgiCiAjaW5jbHVk ZSAiR3JhcGhpY3NDb250ZXh0LmgiCisjaW5jbHVkZSAiSFduZERDLmgiCiAjaW5jbHVkZSAiSW50 UmVjdC5oIgogI2luY2x1ZGUgIk5vdEltcGxlbWVudGVkLmgiCiAjaW5jbHVkZSA8ZDJkMS5oPgpA QCAtMzksMTAgKzQxLDU0IEBACiAKIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAotUmVmUHRyPFVpbnQ4 Q2xhbXBlZEFycmF5PiBJbWFnZUJ1ZmZlckRhdGE6OmdldERhdGEoY29uc3QgSW50UmVjdCYsIGNv bnN0IEludFNpemUmLCBib29sIC8qIGFjY2VsZXJhdGVSZW5kZXJpbmcgKi8sIGJvb2wgLyogdW5t dWx0aXBsaWVkICovLCBmbG9hdCAvKiByZXNvbHV0aW9uU2NhbGUgKi8pIGNvbnN0CitSZWZQdHI8 VWludDhDbGFtcGVkQXJyYXk+IEltYWdlQnVmZmVyRGF0YTo6Z2V0RGF0YShjb25zdCBJbnRSZWN0 JiByZWN0LCBjb25zdCBJbnRTaXplJiBzaXplLCBib29sIC8qIGFjY2VsZXJhdGVSZW5kZXJpbmcg Ki8sIGJvb2wgLyogdW5tdWx0aXBsaWVkICovLCBmbG9hdCAvKiByZXNvbHV0aW9uU2NhbGUgKi8p IGNvbnN0CiB7Ci0gICAgbm90SW1wbGVtZW50ZWQoKTsKLSAgICByZXR1cm4gbnVsbHB0cjsKKyAg ICBhdXRvIHBsYXRmb3JtQ29udGV4dCA9IGNvbnRleHQtPnBsYXRmb3JtQ29udGV4dCgpOworCisg ICAgQ2hlY2tlZDx1bnNpZ25lZCwgUmVjb3JkT3ZlcmZsb3c+IGFyZWEgPSA0OworICAgIGFyZWEg Kj0gcmVjdC53aWR0aCgpOworICAgIGFyZWEgKj0gcmVjdC5oZWlnaHQoKTsKKyAgICBpZiAoYXJl YS5oYXNPdmVyZmxvd2VkKCkpCisgICAgICAgIHJldHVybiBudWxscHRyOworCisgICAgYXV0byBy ZXN1bHQgPSBVaW50OENsYW1wZWRBcnJheTo6Y3JlYXRlVW5pbml0aWFsaXplZChhcmVhLnVuc2Fm ZUdldCgpKTsKKyAgICB1bnNpZ25lZCBjaGFyKiByZXN1bHREYXRhID0gcmVzdWx0ID8gcmVzdWx0 LT5kYXRhKCkgOiBudWxscHRyOworICAgIGlmICghcmVzdWx0RGF0YSkKKyAgICAgICAgcmV0dXJu IG51bGxwdHI7CisKKyAgICBCaXRtYXBJbmZvIGJpdG1hcEluZm8gPSBCaXRtYXBJbmZvOjpjcmVh dGVCb3R0b21VcChzaXplKTsKKworICAgIHZvaWQqIHBpeGVscyA9IG51bGxwdHI7CisgICAgYXV0 byBiaXRtYXAgPSBhZG9wdEdESU9iamVjdCg6OkNyZWF0ZURJQlNlY3Rpb24oMCwgJmJpdG1hcElu Zm8sIERJQl9SR0JfQ09MT1JTLCAmcGl4ZWxzLCAwLCAwKSk7CisKKyAgICBIV25kREMgd2luZG93 REMobnVsbHB0cik7CisgICAgYXV0byBiaXRtYXBEQyA9IGFkb3B0R0RJT2JqZWN0KDo6Q3JlYXRl Q29tcGF0aWJsZURDKHdpbmRvd0RDKSk7CisgICAgSEdESU9CSiBvbGRCaXRtYXAgPSA6OlNlbGVj dE9iamVjdChiaXRtYXBEQy5nZXQoKSwgYml0bWFwLmdldCgpKTsKKworICAgIEhSRVNVTFQgaHI7 CisKKyAgICBDT01QdHI8SUQyRDFHZGlJbnRlcm9wUmVuZGVyVGFyZ2V0PiBnZGlSZW5kZXJUYXJn ZXQ7CisgICAgaHIgPSBwbGF0Zm9ybUNvbnRleHQtPlF1ZXJ5SW50ZXJmYWNlKF9fdXVpZG9mKElE MkQxR2RpSW50ZXJvcFJlbmRlclRhcmdldCksICh2b2lkKiopJmdkaVJlbmRlclRhcmdldCk7Cisg ICAgaWYgKEZBSUxFRChocikpCisgICAgICAgIHJldHVybiBudWxscHRyOworCisgICAgcGxhdGZv cm1Db250ZXh0LT5CZWdpbkRyYXcoKTsKKworICAgIEhEQyBoZGMgPSBudWxscHRyOworICAgIGhy ID0gZ2RpUmVuZGVyVGFyZ2V0LT5HZXREQyhEMkQxX0RDX0lOSVRJQUxJWkVfTU9ERV9DT1BZLCAm aGRjKTsKKworICAgIEJPT0wgb2sgPSA6OkJpdEJsdChiaXRtYXBEQy5nZXQoKSwgMCwgMCwgcmVj dC53aWR0aCgpLCByZWN0LmhlaWdodCgpLCBoZGMsIHJlY3QueCgpLCByZWN0LnkoKSwgU1JDQ09Q WSk7CisKKyAgICBociA9IGdkaVJlbmRlclRhcmdldC0+UmVsZWFzZURDKG51bGxwdHIpOworCisg ICAgaHIgPSBwbGF0Zm9ybUNvbnRleHQtPkVuZERyYXcoKTsKKworICAgIGlmICghb2spCisgICAg ICAgIHJldHVybiBudWxscHRyOworCisgICAgbWVtY3B5KHJlc3VsdC0+ZGF0YSgpLCBwaXhlbHMs IDQgKiByZWN0LndpZHRoKCkgKiByZWN0LmhlaWdodCgpKTsKKworICAgIHJldHVybiByZXN1bHQ7 CiB9CiAKIHZvaWQgSW1hZ2VCdWZmZXJEYXRhOjpwdXREYXRhKFVpbnQ4Q2xhbXBlZEFycmF5KiYg Lyogc291cmNlICovLCBjb25zdCBJbnRTaXplJiAvKiBzb3VyY2VTaXplICovLCBjb25zdCBJbnRS ZWN0JiAvKiBzb3VyY2VSZWN0ICovLCBjb25zdCBJbnRQb2ludCYgLyogZGVzdFBvaW50ICovLCBj b25zdCBJbnRTaXplJiAvKiBzaXplICovLCBib29sIC8qIGFjY2VsZXJhdGVSZW5kZXJpbmcgKi8s IGJvb2wgLyogdW5tdWx0aXBsaWVkICovLCBmbG9hdCAvKiByZXNvbHV0aW9uU2NhbGUgKi8pCklu ZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4vSW1hZ2VCdWZmZXJEaXJl Y3QyRC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mv d2luL0ltYWdlQnVmZmVyRGlyZWN0MkQuY3BwCShyZXZpc2lvbiAyMDc1MjYpCisrKyBTb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4vSW1hZ2VCdWZmZXJEaXJlY3QyRC5jcHAJKHdv cmtpbmcgY29weSkKQEAgLTEwNyw3ICsxMDcsOSBAQCBJbWFnZUJ1ZmZlcjo6SW1hZ2VCdWZmZXIo Y29uc3QgRmxvYXRTaXplCiAgICAgUkVMRUFTRV9BU1NFUlQocmVuZGVyVGFyZ2V0KTsKIAogICAg IENPTVB0cjxJRDJEMUJpdG1hcFJlbmRlclRhcmdldD4gYml0bWFwQ29udGV4dDsKLSAgICBIUkVT VUxUIGhyID0gcmVuZGVyVGFyZ2V0LT5DcmVhdGVDb21wYXRpYmxlUmVuZGVyVGFyZ2V0KEZsb2F0 U2l6ZShtX2xvZ2ljYWxTaXplKSwgJmJpdG1hcENvbnRleHQpOworICAgIEQyRDFfU0laRV9GIGRl c2lyZWRTaXplID0gRmxvYXRTaXplKG1fbG9naWNhbFNpemUpOworICAgIEQyRDFfU0laRV9VIHBp eGVsU2l6ZSA9IEludFNpemUobV9sb2dpY2FsU2l6ZSk7CisgICAgSFJFU1VMVCBociA9IHJlbmRl clRhcmdldC0+Q3JlYXRlQ29tcGF0aWJsZVJlbmRlclRhcmdldCgmZGVzaXJlZFNpemUsICZwaXhl bFNpemUsIG51bGxwdHIsIEQyRDFfQ09NUEFUSUJMRV9SRU5ERVJfVEFSR0VUX09QVElPTlNfR0RJ X0NPTVBBVElCTEUsICZiaXRtYXBDb250ZXh0KTsKICAgICBpZiAoIWJpdG1hcENvbnRleHQgfHwg IVNVQ0NFRURFRChocikpCiAgICAgICAgIHJldHVybjsKIAo=