Bug 32530 - QtWebkit creates an unnecessary deep copy of images when canvas drawing is done
Summary: QtWebkit creates an unnecessary deep copy of images when canvas drawing is done
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: Performance, Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2009-12-14 14:04 PST by Noam Rosenthal
Modified: 2012-02-02 11:00 PST (History)
10 users (show)

See Also:


Attachments
patch (9.63 KB, patch)
2010-02-03 20:11 PST, Ariya Hidayat
no flags Details | Formatted Diff | Diff
patch (9.58 KB, patch)
2010-02-03 21:09 PST, Ariya Hidayat
no flags Details | Formatted Diff | Diff
New proposed patch (2.53 KB, patch)
2010-06-23 07:32 PDT, Samuel Rødal
no flags Details | Formatted Diff | Diff
Resubmitted patch without HTML tags (2.02 KB, patch)
2010-06-23 07:39 PDT, Samuel Rødal
kling: review-
Details | Formatted Diff | Diff
Proposed patch (5.61 KB, patch)
2010-07-16 18:03 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2009-12-14 14:04:11 PST
Found while profiling http://deanm.github.com/pre3d/monster.html with valgrind:
about 50% of the time is spent in image copying, that is called from trying to use the QPixmap copy constructor while a paint operation is active (see qpixmap.cpp, line 275: make a deep copy).
Comment 1 Ariya Hidayat 2010-02-03 20:11:33 PST
Created attachment 48093 [details]
patch
Comment 2 WebKit Review Bot 2010-02-03 20:17:48 PST
Attachment 48093 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/qt/BufferedImageQt.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Ariya Hidayat 2010-02-03 21:09:13 PST
Created attachment 48096 [details]
patch
Comment 4 Simon Hausmann 2010-02-04 02:44:19 PST
Comment on attachment 48096 [details]
patch


> +// BufferedImage just "holds" a reference to the pixmap in the ImageBufferData,
> +// hence we avoid explicit copy (detach) of the QPixmap instance.
> +BufferedImage::BufferedImage(const QPixmap& pixmap)
> +    : m_pixmap(pixmap.pixmapData())

Nice trick to use the private/internal QPixmap constructor ;-)

I wonder however it it's worth it to create an entirely new class for this line of code. Wouldn't it be easier to just make it a _mode_ parameter of the StillImage constructor whether to share or copy the pixmap?
Comment 5 Simon Hausmann 2010-02-04 02:46:15 PST
Alternate idea... I remain a bit worried about using an internal constructor. Technically it's private API, and using private Qt API can quickly become a nightmare for supporting multiple Qt versions.

Would it perhaps make sense to store a QPixmap _pointer_ in the image class instead?

(Instead of an implicit reference, make it explicit)
Comment 6 Ariya Hidayat 2010-02-04 09:51:07 PST
The only reason to have a separate class for this is safety. StillImage is safer to pass around cause QPixmap may copy/detach there, however BufferedImage can only be used within the context of canvas drawing.

Also, I tried to use QPixmap pointer (*) or reference (&) before but it did not help. The reason: QPixmap will still detach (e.g. during the call to QPainter::drawPixmap) is a painter is still active on it or if the painter is activated again (after calling QPainter::end).

This is also the reason why I propose the refactoring of Graphics Context as I outlined in my email to webkit-qt list. Once Graphics Context and Image Buffer "know each other better", we can get rid of this QPixmap dancing because BufferedImage will just push Graphics Context's internal (pixmap) buffer to the screen.

Right now for every single frame, we do the following:
  create pixmap 1, detach/copy to pixmap 2, draw pixmap 2, free pixmap2, free pixmap1

With this patch (the stop-gap measure):
  create pixmap 1, draw pixmap 1, free pixmap 1

And hopefully with my proposal of Graphics Context refactoring:
  draw pixmap from GC buffer

i.e. we keep one pixmap for the entire life of the canvas.
Comment 7 Oliver Hunt 2010-02-04 17:13:46 PST
Comment on attachment 48096 [details]
patch

I'm unsure this is safe -- What happens with this change if you are drawing a canvas onto itself?

I believe ImageBuffer::image() is expected to produce something with effect of a deep copy
Comment 8 Ariya Hidayat 2010-02-04 19:46:46 PST
Good point, Oliver, I hadn't thought about that :(

This means this patch can't be applied. We have to wait for my Qt's Graphics Context refactoring plan.

Also, I agree that image() should be safe with deep-copy. I believe CG will detach if the image is changed. Unlike CG, QPixmap is however "too early" to carry out a deep copy in our situation.

We can solve this once the integration of Graphics Context and Image Buffer is done.
Comment 9 Tor Arne Vestbø 2010-03-05 09:39:36 PST
Please follow the QtWebKit bug reporting guidelines when reporting bugs.

See http://trac.webkit.org/wiki/QtWebKitBugs

Specifically:

  - The 'QtWebKit' component should be used for bugs/features in the public QtWebKit API layer, not to signify that the bug is specific to the Qt port of WebKit

    http://trac.webkit.org/wiki/QtWebKitBugs#Component
Comment 10 Noam Rosenthal 2010-03-05 13:34:20 PST
Right, sorry about that. Posted that bug aq long time ago...
Comment 11 Samuel Rødal 2010-06-23 07:32:17 PDT
Created attachment 59513 [details]
New proposed patch

Simply making m_pixmap a reference solved the problem of deep copying as far as I can see. In addition a check can be made to make sure we don't draw the canvas onto itself without deep copying. I don't see the need to create a new BufferedImage class when StillImage is only used for this single purpose.
Comment 12 Samuel Rødal 2010-06-23 07:39:41 PDT
Created attachment 59514 [details]
Resubmitted patch without HTML tags
Comment 13 Simon Hausmann 2010-06-23 07:46:51 PDT
Comment on attachment 59514 [details]
Resubmitted patch without HTML tags

WebCore/platform/graphics/qt/StillImageQt.h:54
 +          const QPixmap &m_pixmap;
I'm worried that this might introduce crashes.

Can't we store the QPixmap and use isDetached() and data_ptr() to compare the d-pointers?
Comment 14 Samuel Rødal 2010-06-23 08:34:06 PDT
(In reply to comment #13)
> (From update of attachment 59514 [details])
> WebCore/platform/graphics/qt/StillImageQt.h:54
>  +          const QPixmap &m_pixmap;
> I'm worried that this might introduce crashes.
> 
> Can't we store the QPixmap and use isDetached() and data_ptr() to compare the d-pointers?

The problem is the QPixmap copy constructor which does the following:

    if (pixmap.paintingActive()) {                // make a deep copy
        operator=(pixmap.copy());
    } else {
        data = pixmap.data;
    }

And for the ImageBuffer pixmap used by Canvas a painter is _always_ active. So we can't even create a shallow copy of the QPixmap, as that will cause a deep copy, instead we need to keep a reference and do the copying ourselves when it's needed.
Comment 15 Andreas Kling 2010-07-02 14:57:44 PDT
Comment on attachment 59514 [details]
Resubmitted patch without HTML tags

Clever. I might've added a comment, but meh. LGTM :)
Comment 16 Kenneth Rohde Christiansen 2010-07-02 15:05:05 PDT
(In reply to comment #15)
> (From update of attachment 59514 [details])
> Clever. I might've added a comment, but meh. LGTM :)

Yes, a comment in the code to this "unusual construct" might be good.
Comment 17 Simon Hausmann 2010-07-03 15:27:36 PDT
Comment on attachment 59514 [details]
Resubmitted patch without HTML tags

Ok, with the way the StillImage is allocated and kept alive along with m_data.m_pixmap I agree that this can work.

However I think this patch breaks the other use of StillImage in ImageQt.cpp, where StillImage::create() is called with a temporary QPixmap. With your change StillImage holds only a reference to the pixmap, which in that case will be a dangling reference.

What about adding API to QPixmap to support this use-case?
How about using the internal QPixmap constructor that takes a QPixmapData and somehow increasing the refcount?
(you know that the returned d-pointer is a sub-class of QSharedData... ;-)
Comment 18 Andreas Kling 2010-07-07 15:19:14 PDT
Comment on attachment 59514 [details]
Resubmitted patch without HTML tags

(In reply to comment #17)
> However I think this patch breaks the other use of StillImage in ImageQt.cpp, where StillImage::create() is called with a temporary QPixmap. With your change StillImage holds only a reference to the pixmap, which in that case will be a dangling reference.

You are right.

> What about adding API to QPixmap to support this use-case?

AFAICT this is the only way out. The latest patch here will break behavior, for example patterns created from a canvas element. They must retain an image of the canvas contents at the time of pattern creation (this is tested by LayoutTests/fast/canvas/canvas-pattern-modify.html)

r- for breaking behavior. Would love this improvement though.
Comment 19 Andreas Kling 2010-07-16 18:00:29 PDT
This could be fixed by adding a method to ImageBuffer that returns an image that can be used for rendering now, but shouldn't be kept around. Patch coming..
Comment 20 Andreas Kling 2010-07-16 18:03:42 PDT
Created attachment 61866 [details]
Proposed patch

Image* ImageBuffer::imageForRendering()

This method returns an image that can be used for rendering right now (i.e the QPixmap backing our canvas rendering context.) This lets us avoid the QPixmap copy due to a painter always being active.

For all other ports, it simply returns ImageBuffer::image();
Comment 21 Oliver Hunt 2010-07-16 20:06:07 PDT
Comment on attachment 61866 [details]
Proposed patch

I don't see how this helps -- What happens when i do

canvas.getContext("2d").drawImage(canvas, 0, 0)

?

Afaict you still end up writing to your source image before reading from it.
Comment 22 Andreas Kling 2010-07-16 20:11:06 PDT
(In reply to comment #21)
> I don't see how this helps -- What happens when i do
> canvas.getContext("2d").drawImage(canvas, 0, 0)
> ?
> Afaict you still end up writing to your source image before reading from it.

In CanvasRenderingContext2D::drawImage(HTMLCanvasElement*, ...):

c->drawImage(buffer->image(), DeviceColorSpace, destRect, sourceRect, state().m_globalComposite);

buffer->image() here will create a copy of the source canvas's backing image and paint that onto the canvas.

Or am I missing something?
Comment 23 Oliver Hunt 2010-07-16 20:13:22 PDT
Comment on attachment 61866 [details]
Proposed patch

Ah, I see.  r=me
Comment 24 Andreas Kling 2010-07-16 20:23:18 PDT
Comment on attachment 61866 [details]
Proposed patch

Clearing flags on attachment: 61866

Committed r63606: <http://trac.webkit.org/changeset/63606>
Comment 25 Andreas Kling 2010-07-16 20:23:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Noam Rosenthal 2012-02-02 10:51:54 PST
This has regressed a long time ago. We make a deep copy of canvas with every paint again, since some changes to ImageBuffer has been made. Should I reopen this bug or open a new one?
Comment 27 Antonio Gomes 2012-02-02 11:00:39 PST
(In reply to comment #26)
> This has regressed a long time ago. We make a deep copy of canvas with every paint again, since some changes to ImageBuffer has been made. Should I reopen this bug or open a new one?

new one, please :)