Avoid extra copy when drawing images
Created attachment 216687 [details] Patch
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 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))
Created attachment 216980 [details] Patch
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.
Created attachment 216982 [details] Patch
Comment on attachment 216982 [details] Patch Clearing flags on attachment: 216982 Committed r159314: <http://trac.webkit.org/changeset/159314>
All reviewed patches have been landed. Closing bug.
(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 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()));
(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?
(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.
I've merged the correct patch in the stable branch see http://trac.webkit.org/changeset/160081
(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());
Created attachment 218485 [details] Patch
(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 on attachment 218485 [details] Patch This is leaking the surface, please check what I did in the stable branch.
Created attachment 218515 [details] Patch
Comment on attachment 218515 [details] Patch Thanks!
Comment on attachment 218515 [details] Patch Clearing flags on attachment: 218515 Committed r160177: <http://trac.webkit.org/changeset/160177>