Bug 32530 – QtWebkit creates an unnecessary deep copy of images when canvas drawing is done

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

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).

[Ariya Hidayat]

Created attachment 48093 [details]
patch

[WebKit Review Bot]

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.

[Ariya Hidayat]

Created attachment 48096 [details]
patch

[Simon Hausmann]

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?

[Simon Hausmann]

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)

[Ariya Hidayat]

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.

[Oliver Hunt]

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

[Ariya Hidayat]

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.

[Tor Arne Vestbø]

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

[Noam Rosenthal]

Right, sorry about that. Posted that bug aq long time ago...

[Samuel Rødal]

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.

[Samuel Rødal]

Created attachment 59514 [details]
Resubmitted patch without HTML tags

[Simon Hausmann]

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?

[Samuel Rødal]

(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.

[Andreas Kling]

Comment on attachment 59514 [details]
Resubmitted patch without HTML tags

Clever. I might've added a comment, but meh. LGTM :)

[Kenneth Rohde Christiansen]

(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.

[Simon Hausmann]

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... ;-)

[Andreas Kling]

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.

[Andreas Kling]

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..

[Andreas Kling]

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();

[Oliver Hunt]

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.

[Andreas Kling]

(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?

[Oliver Hunt]

Comment on attachment 61866 [details]
Proposed patch

Ah, I see.  r=me

[Andreas Kling]

Comment on attachment 61866 [details]
Proposed patch

Clearing flags on attachment: 61866

Committed r63606: <http://trac.webkit.org/changeset/63606>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

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?

[Antonio Gomes]

(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 :)