89865 2012-06-25 04:07:15 -0700 [Qt] Avoid a deep copy of QImage in GraphicsContext3D::getImageData. 2012-06-25 19:29:14 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 dongseong.hwang webkit-unassigned noam webkit.review.bot oldest_to_newest 656466 0 dongseong.hwang 2012-06-25 04:07:15 -0700 [Qt] Avoid a deep copy of QImage in GraphicsContext3D::getImageData. 656467 1 149270 dongseong.hwang 2012-06-25 04:10:25 -0700 Created attachment 149270 patch This patch follows BitmapTextureGL::updateContents. void BitmapTextureGL::updateContents(Image* image, const IntRect& targetRect, const IntPoint& offset) { if (!image) return; NativeImagePtr frameImage = image->nativeImageForCurrentFrame(); if (!frameImage) return; int bytesPerLine; const char* imageData; #if PLATFORM(QT) QImage qtImage; #if HAVE(QT5) // With QPA, we can avoid a deep copy. qtImage = *frameImage->handle()->buffer(); #else // This might be a deep copy, depending on other references to the pixmap. qtImage = frameImage->toImage(); #endif imageData = reinterpret_cast<const char*>(qtImage.constBits()); bytesPerLine = qtImage.bytesPerLine(); #elif USE(CAIRO) cairo_surface_t* surface = frameImage->surface(); imageData = reinterpret_cast<const char*>(cairo_image_surface_get_data(surface)); bytesPerLine = cairo_image_surface_get_stride(surface); #endif updateContents(imageData, targetRect, offset, bytesPerLine); } 656803 2 149270 noam 2012-06-25 13:06:47 -0700 Comment on attachment 149270 patch While we're at it, we should convert the format to Format_ARGB32_Premultiplied if premultiplied is requested, instead of changing to ARGB32 and then premultiplying as an additional step. 657050 3 dongseong.hwang 2012-06-25 17:22:34 -0700 Would you mind explain more detail? I don't fully understand. As I understand, you means the two code stubs are different? 1. Original nativeImage = nativePixmap->toImage().convertToFormat(QImage::Format_ARGB32); 2. Changed (For Qt4) QImage qtImage; qtImage = nativePixmap->toImage(); nativeImage = qtImage.convertToFormat(QImage::Format_ARGB32); If the two is different and original stub is correct, can I change like this? - nativeImage = nativePixmap->toImage().convertToFormat(QImage::Format_ARGB32); +#if HAVE(QT5) + // With QPA, we can avoid a deep copy. + nativeImage = nativePixmap->handle()->buffer()->convertToFormat(QImage::Format_ARGB32); +#else + // This might be a deep copy, depending on other references to the pixmap. + nativeImage = nativePixmap->toImage().convertToFormat(QImage::Format_ARGB32); +#endif 657107 4 149270 noam 2012-06-25 18:16:01 -0700 Comment on attachment 149270 patch There is an additional optimization to do, but we should do it in a different patch. 657130 5 149270 webkit.review.bot 2012-06-25 18:30:35 -0700 Comment on attachment 149270 patch Clearing flags on attachment: 149270 Committed r121209: <http://trac.webkit.org/changeset/121209> 657131 6 webkit.review.bot 2012-06-25 18:30:39 -0700 All reviewed patches have been landed. Closing bug. 657186 7 noam 2012-06-25 19:29:14 -0700 Created a new bug for the proposed additional optimization, https://bugs.webkit.org/show_bug.cgi?id=89937 149270 2012-06-25 04:10:25 -0700 2012-06-25 18:30:35 -0700 patch 0001-Qt-Avoid-a-deep-copy-of-QImage-in-GraphicsContext3D-.patch text/plain 2505 dongseong.hwang RnJvbSAxZjkyOGRjMWI3NTYxZTQ0NWZiOWE5MTBkMGM0MWI5NWM3MmM5NjcyIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBIdWFuZyBEb25nc3VuZyA8bHV4dGVsbGFAY29tcGFueTEwMC5u ZXQ+CkRhdGU6IE1vbiwgMjUgSnVuIDIwMTIgMTk6MDk6MTcgKzA5MDAKU3ViamVjdDogW1BBVENI XSBbUXRdIEF2b2lkIGEgZGVlcCBjb3B5IG9mIFFJbWFnZSBpbgogR3JhcGhpY3NDb250ZXh0M0Q6 OmdldEltYWdlRGF0YS4KClRoaXMgcGF0Y2ggZm9sbG93cyBCaXRtYXBUZXh0dXJlR0w6OnVwZGF0 ZUNvbnRlbnRzLgotLS0KIFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyAgICAgICAgICAgICAgICAg ICAgICAgICAgIHwgICAxMiArKysrKysrKysrKysKIC4uLi9wbGF0Zm9ybS9ncmFwaGljcy9xdC9H cmFwaGljc0NvbnRleHQzRFF0LmNwcCAgIHwgICAxMSArKysrKysrKysrLQogMiBmaWxlcyBjaGFu Z2VkLCAyMiBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0pCgpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYkNvcmUvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCmluZGV4IGMwNmNk MjAuLjYzNTgzNTk1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIv U291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTItMDYtMjUgIEh1 YW5nIERvbmdzdW5nICA8bHV4dGVsbGFAY29tcGFueTEwMC5uZXQ+CisKKyAgICAgICAgW1F0XSBB dm9pZCBhIGRlZXAgY29weSBvZiBRSW1hZ2UgaW4gR3JhcGhpY3NDb250ZXh0M0Q6OmdldEltYWdl RGF0YS4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTg5 ODY1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm8g bmV3IHRlc3RzLiBDb3ZlcmVkIGJ5IGV4aXN0aW5nIHRlc3RzLgorCisgICAgICAgICogcGxhdGZv cm0vZ3JhcGhpY3MvcXQvR3JhcGhpY3NDb250ZXh0M0RRdC5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpHcmFwaGljc0NvbnRleHQzRDo6Z2V0SW1hZ2VEYXRhKToKKwogMjAxMi0wNi0yNCAgRnVtaXRv c2hpIFVrYWkgIDx1a2FpQGNocm9taXVtLm9yZz4KIAogICAgICAgICBVbnJldmlld2VkIGNvbXBp bGUgZXJyb3IgZml4IG9mIENocm9taXVtIFdpbiBSZWxlYXNlLgpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvcXQvR3JhcGhpY3NDb250ZXh0M0RRdC5jcHAgYi9T b3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9xdC9HcmFwaGljc0NvbnRleHQzRFF0LmNw cAppbmRleCAyZjUyNjIxLi4zN2IxODE3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9ncmFwaGljcy9xdC9HcmFwaGljc0NvbnRleHQzRFF0LmNwcAorKysgYi9Tb3VyY2UvV2Vi Q29yZS9wbGF0Zm9ybS9ncmFwaGljcy9xdC9HcmFwaGljc0NvbnRleHQzRFF0LmNwcApAQCAtMzUs NiArMzUsNyBAQAogI2luY2x1ZGUgIlNoYXJlZEJ1ZmZlci5oIgogI2lmIEhBVkUoUVQ1KQogI2lu Y2x1ZGUgPFFXaW5kb3c+CisjaW5jbHVkZSA8cXBhL3FwbGF0Zm9ybXBpeG1hcC5oPgogI2VuZGlm CiAjaW5jbHVkZSA8d3RmL1VudXNlZFBhcmFtLmg+CiAjaW5jbHVkZSA8d3RmL3RleHQvQ1N0cmlu Zy5oPgpAQCAtNDkwLDcgKzQ5MSwxNSBAQCBib29sIEdyYXBoaWNzQ29udGV4dDNEOjpnZXRJbWFn ZURhdGEoSW1hZ2UqIGltYWdlLAogICAgICAgICBuYXRpdmVJbWFnZSA9IFFJbWFnZTo6ZnJvbURh dGEocmVpbnRlcnByZXRfY2FzdDxjb25zdCB1Y2hhcio+KGltYWdlLT5kYXRhKCktPmRhdGEoKSks IGltYWdlLT5kYXRhKCktPnNpemUoKSkuY29udmVydFRvRm9ybWF0KFFJbWFnZTo6Rm9ybWF0X0FS R0IzMik7CiAgICAgZWxzZSB7CiAgICAgICAgIFFQaXhtYXAqIG5hdGl2ZVBpeG1hcCA9IGltYWdl LT5uYXRpdmVJbWFnZUZvckN1cnJlbnRGcmFtZSgpOwotICAgICAgICBuYXRpdmVJbWFnZSA9IG5h dGl2ZVBpeG1hcC0+dG9JbWFnZSgpLmNvbnZlcnRUb0Zvcm1hdChRSW1hZ2U6OkZvcm1hdF9BUkdC MzIpOworICAgICAgICBRSW1hZ2UgcXRJbWFnZTsKKyNpZiBIQVZFKFFUNSkKKyAgICAgICAgLy8g V2l0aCBRUEEsIHdlIGNhbiBhdm9pZCBhIGRlZXAgY29weS4KKyAgICAgICAgcXRJbWFnZSA9ICpu YXRpdmVQaXhtYXAtPmhhbmRsZSgpLT5idWZmZXIoKTsKKyNlbHNlCisgICAgICAgIC8vIFRoaXMg bWlnaHQgYmUgYSBkZWVwIGNvcHksIGRlcGVuZGluZyBvbiBvdGhlciByZWZlcmVuY2VzIHRvIHRo ZSBwaXhtYXAuCisgICAgICAgIHF0SW1hZ2UgPSBuYXRpdmVQaXhtYXAtPnRvSW1hZ2UoKTsKKyNl bmRpZgorICAgICAgICBuYXRpdmVJbWFnZSA9IHF0SW1hZ2UuY29udmVydFRvRm9ybWF0KFFJbWFn ZTo6Rm9ybWF0X0FSR0IzMik7CiAgICAgfQogICAgIEFscGhhT3AgbmVlZGVkQWxwaGFPcCA9IEFs cGhhRG9Ob3RoaW5nOwogICAgIGlmIChwcmVtdWx0aXBseUFscGhhKQotLSAKMS43LjkuNQoK