Bug 124209 - [Cairo] Avoid extra copy when drawing images
Summary: [Cairo] Avoid extra copy when drawing images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-12 09:19 PST by Aloisio Almeida Jr
Modified: 2013-12-05 09:15 PST (History)
9 users (show)

See Also:


Attachments
Patch (4.38 KB, patch)
2013-11-12 09:32 PST, Aloisio Almeida Jr
no flags Details | Formatted Diff | Diff
Patch (4.28 KB, patch)
2013-11-14 14:25 PST, Aloisio Almeida Jr
no flags Details | Formatted Diff | Diff
Patch (4.29 KB, patch)
2013-11-14 14:37 PST, Aloisio Almeida Jr
no flags Details | Formatted Diff | Diff
Patch (2.72 KB, patch)
2013-12-04 20:01 PST, Aloisio Almeida Jr
no flags Details | Formatted Diff | Diff
Patch (3.06 KB, patch)
2013-12-05 08:10 PST, Aloisio Almeida Jr
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aloisio Almeida Jr 2013-11-12 09:19:33 PST
Avoid extra copy when drawing images
Comment 1 Aloisio Almeida Jr 2013-11-12 09:32:56 PST
Created attachment 216687 [details]
Patch
Comment 2 Aloisio Almeida Jr 2013-11-12 09:45:21 PST
Considering the image benchmark from: http://flashcanvas.net/examples/dl.dropbox.com/u/1865210/mindcat/canvas_perf.html the proposed patch has the following results:

Nix: ~15% gain;
GTK: ~10% gain;
EFL: ~10% gain;

We also did tests with Nix using image low interpolation quality. The gain was ~50%.
Comment 3 Martin Robinson 2013-11-14 11:03:08 PST
Comment on attachment 216687 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216687&action=review

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:181
> +    if (srcRect.x() || srcRect.y()
> +        || srcRect.width() != cairo_image_surface_get_width(surface)
> +        || srcRect.height() != cairo_image_surface_get_height(surface)) {

There is no guarantee that this surface is an image surface. You could simplify this check to be:
if (srcRect.x() || srcRect.y() || srcRect.size() == cairoSurfaceSize(surface))
Comment 4 Aloisio Almeida Jr 2013-11-14 14:25:55 PST
Created attachment 216980 [details]
Patch
Comment 5 Martin Robinson 2013-11-14 14:32:44 PST
Comment on attachment 216980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216980&action=review

Thanks.

> Source/WebCore/ChangeLog:3
> +        Avoid extra copy when drawing images

The actual bug title is different, so make sure these match up when landing.
Comment 6 Aloisio Almeida Jr 2013-11-14 14:37:00 PST
Created attachment 216982 [details]
Patch
Comment 7 WebKit Commit Bot 2013-11-14 15:13:12 PST
Comment on attachment 216982 [details]
Patch

Clearing flags on attachment: 216982

Committed r159314: <http://trac.webkit.org/changeset/159314>
Comment 8 WebKit Commit Bot 2013-11-14 15:13:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Carlos Garcia Campos 2013-12-02 00:21:09 PST
(In reply to comment #3)
> (From update of attachment 216687 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216687&action=review
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:181
> > +    if (srcRect.x() || srcRect.y()
> > +        || srcRect.width() != cairo_image_surface_get_width(surface)
> > +        || srcRect.height() != cairo_image_surface_get_height(surface)) {
> 
> There is no guarantee that this surface is an image surface. You could simplify this check to be:
> if (srcRect.x() || srcRect.y() || srcRect.size() == cairoSurfaceSize(surface))

Didn't you actually mean srcRect.x() || srcRect.y() || srcRect.size() != cairoSurfaceSize(surface)? The previous expression checked there's x or y or size is different.
Comment 10 Carlos Garcia Campos 2013-12-02 00:25:03 PST
Comment on attachment 216982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216982&action=review

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:176
> +    RefPtr<cairo_surface_t> subsurface;

This doesn't seem to be used outside the if context, I think it could be moved to the if block.

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:179
> +    if (srcRect.x() || srcRect.y() || srcRect.size() == cairoSurfaceSize(surface)) {

If I understood correctly the commit message, you want to use a subsurface when source size is different to surface size, right?

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:187
> +        subsurface = adoptRef(cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(),
> +            expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height()));
> +        patternSurface = subsurface.get();

Maybe you don't even need to subsurface variable if you use a single lines here:

patternSurface = adoptRef(cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(), expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height()));
Comment 11 Aloisio Almeida Jr 2013-12-02 07:26:10 PST
(In reply to comment #10)
> (From update of attachment 216982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216982&action=review
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:176
> > +    RefPtr<cairo_surface_t> subsurface;
> 
> This doesn't seem to be used outside the if context, I think it could be moved to the if block.
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:179
> > +    if (srcRect.x() || srcRect.y() || srcRect.size() == cairoSurfaceSize(surface)) {
> 
> If I understood correctly the commit message, you want to use a subsurface when source size is different to surface size, right?

Right. I got the line from Robinson review without double check, my bad.

> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:187
> > +        subsurface = adoptRef(cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(),
> > +            expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height()));
> > +        patternSurface = subsurface.get();
> 
> Maybe you don't even need to subsurface variable if you use a single lines here:
> 
> patternSurface = adoptRef(cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(), expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height()));

Ok.

The question is: the patch has already landed. How to proceed?
Comment 12 Carlos Garcia Campos 2013-12-02 09:16:41 PST
(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 216982 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=216982&action=review
> > 
> > > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:176
> > > +    RefPtr<cairo_surface_t> subsurface;
> > 
> > This doesn't seem to be used outside the if context, I think it could be moved to the if block.
> > 
> > > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:179
> > > +    if (srcRect.x() || srcRect.y() || srcRect.size() == cairoSurfaceSize(surface)) {
> > 
> > If I understood correctly the commit message, you want to use a subsurface when source size is different to surface size, right?
> 
> Right. I got the line from Robinson review without double check, my bad.
> 
> > 
> > > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:187
> > > +        subsurface = adoptRef(cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(),
> > > +            expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height()));
> > > +        patternSurface = subsurface.get();
> > 
> > Maybe you don't even need to subsurface variable if you use a single lines here:
> > 
> > patternSurface = adoptRef(cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(), expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height()));
> 
> Ok.
> 
> The question is: the patch has already landed. How to proceed?

I don't think we need to roll it out, since it hasn't broken anything, I'm reopening the bug and you can just upload a new patch.
Comment 13 Carlos Garcia Campos 2013-12-04 01:54:12 PST
I've merged the correct patch in the stable branch see http://trac.webkit.org/changeset/160081
Comment 14 Aloisio Almeida Jr 2013-12-04 13:41:05 PST
(In reply to comment #10)
> (From update of attachment 216982 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216982&action=review
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:176
> > +    RefPtr<cairo_surface_t> subsurface;
> 
> This doesn't seem to be used outside the if context, I think it could be moved to the if block.
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:179
> > +    if (srcRect.x() || srcRect.y() || srcRect.size() == cairoSurfaceSize(surface)) {
> 
> If I understood correctly the commit message, you want to use a subsurface when source size is different to surface size, right?
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:187
> > +        subsurface = adoptRef(cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(),
> > +            expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height()));
> > +        patternSurface = subsurface.get();
> 
> Maybe you don't even need to subsurface variable if you use a single lines here:
> 
> patternSurface = adoptRef(cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(), expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height()));


The pattern will hold the subsurface reference. The subsurface will be destroyed together with the pattern. So, I don't need to hold the subsurface reference as a RefPtr.

I will remove the subsurface variable and use:

patternSurface = cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(), expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height());
Comment 15 Aloisio Almeida Jr 2013-12-04 20:01:56 PST
Created attachment 218485 [details]
Patch
Comment 16 Carlos Garcia Campos 2013-12-04 23:31:28 PST
(In reply to comment #14)
> (In reply to comment #10)
> > (From update of attachment 216982 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=216982&action=review
> > 
> > > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:176
> > > +    RefPtr<cairo_surface_t> subsurface;
> > 
> > This doesn't seem to be used outside the if context, I think it could be moved to the if block.
> > 
> > > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:179
> > > +    if (srcRect.x() || srcRect.y() || srcRect.size() == cairoSurfaceSize(surface)) {
> > 
> > If I understood correctly the commit message, you want to use a subsurface when source size is different to surface size, right?
> > 
> > > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:187
> > > +        subsurface = adoptRef(cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(),
> > > +            expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height()));
> > > +        patternSurface = subsurface.get();
> > 
> > Maybe you don't even need to subsurface variable if you use a single lines here:
> > 
> > patternSurface = adoptRef(cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(), expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height()));
> 
> 
> The pattern will hold the subsurface reference. The subsurface will be destroyed together with the pattern. So, I don't need to hold the subsurface reference as a RefPtr.

No, the pattern doesn't adopt the surface reference, but increases it.

> I will remove the subsurface variable and use:
> 
> patternSurface = cairo_surface_create_for_rectangle(surface, expandedSrcRect.x(), expandedSrcRect.y(), expandedSrcRect.width(), expandedSrcRect.height());

That leaks the surface, when the pattern is destroyed the surface ref counter will still be 1
Comment 17 Carlos Garcia Campos 2013-12-04 23:32:15 PST
Comment on attachment 218485 [details]
Patch

This is leaking the surface, please check what I did in the stable branch.
Comment 18 Aloisio Almeida Jr 2013-12-05 08:10:07 PST
Created attachment 218515 [details]
Patch
Comment 19 Carlos Garcia Campos 2013-12-05 08:27:35 PST
Comment on attachment 218515 [details]
Patch

Thanks!
Comment 20 WebKit Commit Bot 2013-12-05 09:15:30 PST
Comment on attachment 218515 [details]
Patch

Clearing flags on attachment: 218515

Committed r160177: <http://trac.webkit.org/changeset/160177>
Comment 21 WebKit Commit Bot 2013-12-05 09:15:34 PST
All reviewed patches have been landed.  Closing bug.