Bug 42945

Summary: [Qt] getImageData(): Single-pass RGB->BGR and un-premultiplication
Product: WebKit Reporter: Andreas Kling <kling>
Component: WebCore Misc.Assignee: QtWebKit Unassigned <webkit-qt-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: amandatheloves, ap, ashwinkumar9944, eilidhabbott, roger9an0, tbt19901508, tuborgmirch
Priority: P2 Keywords: Performance, Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
kling: review-
Proposed patch v2 none

Description Andreas Kling 2010-07-25 04:12:27 PDT
When retrieving the unmultiplied pixel data via CanvasRenderingContext2D's getImageData(), QtWebKit currently does the following:

1. QPixmap::toImage() (our canvas is backed by a QPixmap)
2. QImage::convertToFormat(ARGB32)
3. Copy image byte-by-byte to destination buffer swapping Red and Blue channels.

Benchmarking with http://www.semantix.gr/statue.html I get these numbers before any changes:

23.43 FPS { 23.52, 23.49, 23.41, 23.46, 23.48, 23.35, 23.33 }

We can optimize this as such:

1. QPixmap::toImage()
2. QImage::convertToFormat(ARGB32_PM) (no-op on raster paintengine)
3. Copy image pixel by pixel to destination buffer doing combined RGB->BGR and un-premultiplication.

Same benchmark with the above algorithm:

25.19 FPS { 25.15, 25.22, 25.32, 25.31, 25.03, 25.07, 25.23 }
Comment 1 Andreas Kling 2010-07-25 04:21:46 PDT
Created attachment 62523 [details]
Proposed patch
Comment 2 Kenneth Rohde Christiansen 2010-07-25 07:17:43 PDT
Comment on attachment 62523 [details]
Proposed patch

WebCore/platform/graphics/qt/ImageBufferQt.cpp:178
 +      QImage image = imageData.m_pixmap.toImage().convertToFormat(QImage::Format_ARGB32_Premultiplied);
I think it would be nice with a comment here why you disconsider the bool multiplied, people might think that is an error in the future.

WebCore/platform/graphics/qt/ImageBufferQt.cpp:212
 +                  pixel = ((pixel << 16) & 0xff0000) | ((pixel >> 16) & 0xff) | (pixel & 0xff00ff00);
Maybe you should explain the RGB->BGR swapping here, just like a little comment
Comment 3 Andreas Kling 2010-07-25 16:56:34 PDT
Comment on attachment 62523 [details]
Proposed patch

r- due to comments from John Brooks on IRC; this code should have a special path for opaque colors.
Comment 4 Andreas Kling 2010-07-26 04:30:27 PDT
Created attachment 62557 [details]
Proposed patch v2

Added special path for un-premultiplication when alpha == 255.
Also added comments like Kenneth requested.
Comment 5 Andreas Kling 2010-07-26 04:51:51 PDT
Comment on attachment 62557 [details]
Proposed patch v2

Clearing flags on attachment: 62557

Committed r64041: <http://trac.webkit.org/changeset/64041>
Comment 6 Andreas Kling 2010-07-26 04:52:02 PDT
All reviewed patches have been landed.  Closing bug.