Bug 118808 - [cairo] canvas drawing on itself doesn't work with accelerated canvas
Summary: [cairo] canvas drawing on itself doesn't work with accelerated canvas
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: arno.
URL: http://renevier.net/tmp/drawImage.sel...
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-17 13:17 PDT by arno.
Modified: 2013-08-29 10:22 PDT (History)
13 users (show)

See Also:


Attachments
patch proposal (8.22 KB, patch)
2013-07-17 17:20 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch: guard with ENABLE(ACCELERATED_2D_CANVAS) when needed (8.28 KB, patch)
2013-07-18 12:27 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch (15.17 KB, patch)
2013-07-22 16:44 PDT, arno.
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (1.23 MB, application/zip)
2013-07-22 21:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from APPLE-EWS-9 for win-future (259.50 KB, application/zip)
2013-07-27 12:42 PDT, Build Bot
no flags Details
updated patch: extractImage still didn't work when getting data from an accelerated canvas (15.89 KB, patch)
2013-08-01 16:54 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch (16.25 KB, patch)
2013-08-12 12:35 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch (16.40 KB, patch)
2013-08-27 15:09 PDT, arno.
no flags Details | Formatted Diff | Diff
updated patch (16.63 KB, patch)
2013-08-27 18:08 PDT, arno.
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description arno. 2013-07-17 13:17:18 PDT
Hi, when drawing a canvas on itself with accelerated canvas, nothing is drawn
See test url for an example
Comment 1 arno. 2013-07-17 17:20:38 PDT
Created attachment 206931 [details]
patch proposal
Comment 2 EFL EWS Bot 2013-07-17 17:45:59 PDT
Comment on attachment 206931 [details]
patch proposal

Attachment 206931 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1100316
Comment 3 EFL EWS Bot 2013-07-17 18:33:10 PDT
Comment on attachment 206931 [details]
patch proposal

Attachment 206931 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1103370
Comment 4 arno. 2013-07-18 12:27:31 PDT
Created attachment 207013 [details]
updated patch: guard with ENABLE(ACCELERATED_2D_CANVAS) when needed
Comment 5 Chris Dumez 2013-07-19 03:21:35 PDT
Comment on attachment 207013 [details]
updated patch: guard with ENABLE(ACCELERATED_2D_CANVAS) when needed

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

> Source/WebCore/ChangeLog:21
> +        No new tests. Covered by existing tests.

So which tests change output? Why aren't they unskipped / rebaselined in the same patch?
Comment 6 arno. 2013-07-19 08:47:02 PDT
(In reply to comment #5)
> (From update of attachment 207013 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207013&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        No new tests. Covered by existing tests.
> 
> So which tests change output? Why aren't they unskipped / rebaselined in the same patch?

I ran the tests with accelerated canvas enabled (and minimumAccelerated2dCanvasSize set to 1x1).

LayoutTests/canvas/philip/tests/2d.drawImage.self.1.html changes
Comment 7 Martin Robinson 2013-07-19 08:50:57 PDT
(In reply to comment #5)
> (From update of attachment 207013 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207013&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        No new tests. Covered by existing tests.
> 
> So which tests change output? Why aren't they unskipped / rebaselined in the same patch?

Accelerated canvas is not enabled on the bots.
Comment 8 arno. 2013-07-22 12:13:53 PDT
Comment on attachment 207013 [details]
updated patch: guard with ENABLE(ACCELERATED_2D_CANVAS) when needed

I found some other places which wrongfully expect an image.
Comment 9 arno. 2013-07-22 16:44:30 PDT
Created attachment 207293 [details]
updated patch
Comment 10 Build Bot 2013-07-22 21:35:51 PDT
Comment on attachment 207293 [details]
updated patch

Attachment 207293 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1184058

New failing tests:
editing/unsupported-content/table-delete-003.html
editing/unsupported-content/list-type-before.html
editing/unsupported-content/table-type-before.html
editing/unsupported-content/list-delete-003.html
editing/unsupported-content/list-delete-001.html
editing/unsupported-content/list-type-after.html
editing/unsupported-content/table-delete-002.html
editing/unsupported-content/table-type-after.html
editing/unsupported-content/table-delete-001.html
Comment 11 Build Bot 2013-07-22 21:35:54 PDT
Created attachment 207302 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3
Comment 12 Build Bot 2013-07-27 12:42:29 PDT
Comment on attachment 207293 [details]
updated patch

Attachment 207293 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1247526

New failing tests:
dom/xhtml/level2/events/dispatchEvent03.xhtml
dom/svg/level3/xpath/XPathEvaluator_evaluate_INVALID_EXPRESSION_ERR.svg
dom/html/level2/events/dispatchEvent04.html
dom/xhtml/level2/events/dispatchEvent02.xhtml
dom/svg/level3/xpath/XPathEvaluator_createExpression_INVALID_EXPRESSION_ERR.svg
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_02.svg
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref.html
dom/svg/level3/xpath/XPathEvaluator_evaluate_NOT_SUPPORTED_ERR.svg
dom/xhtml/level2/events/dispatchEvent04.xhtml
dom/svg/level3/xpath/XPathEvaluator_evaluate_NAMESPACE_ERR.svg
dom/html/level2/events/dispatchEvent01.html
dom/xhtml/level2/events/dispatchEvent01.xhtml
dom/html/level1/core/hc_attrgetvalue2.html
dom/html/level2/events/dispatchEvent03.html
dom/html/level2/events/dispatchEvent02.html
dom/html/level2/core/createDocumentType04.html
dom/xhtml/level2/events/dispatchEvent05.xhtml
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref1.html
dom/html/level1/core/hc_attrappendchild2.html
dom/html/level2/events/dispatchEvent06.html
css1/basic/comments.html
dom/html/level2/events/dispatchEvent07.html
dom/html/level2/core/setAttributeNS10.html
dom/html/level2/events/dispatchEvent05.html
dom/html/level2/core/createAttributeNS06.html
dom/html/level1/core/hc_attrappendchild4.html
dom/svg/level3/xpath/XPathEvaluator_createExpression_NAMESPACE_ERR_01.svg
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi1.html
dom/html/level1/core/documentinvalidcharacterexceptioncreatepi.html
dom/html/level2/core/hc_namednodemapinvalidtype1.html
Comment 13 Build Bot 2013-07-27 12:42:33 PDT
Created attachment 207588 [details]
Archive of layout-test-results from APPLE-EWS-9 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-9  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit
Comment 14 arno. 2013-08-01 16:54:50 PDT
Created attachment 207969 [details]
updated patch: extractImage still didn't work when getting data from an accelerated canvas
Comment 15 Martin Robinson 2013-08-07 10:58:22 PDT
Comment on attachment 207969 [details]
updated patch: extractImage still didn't work when getting data from an accelerated canvas

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

Looks pretty good, just a few minor points.

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:224
> +    RefPtr<cairo_surface_t> newSurface = adoptRef(cairo_surface_create_similar(originalSurface,
> +        cairo_surface_get_content(originalSurface),
> +        size.width(), size.height()));

This can be one line.

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:267
> +    int width = 0;
> +    int height = 0;
> +    switch (cairo_surface_get_type(surface)) {
> +    case CAIRO_SURFACE_TYPE_IMAGE:
> +        width = cairo_image_surface_get_width(surface);
> +        height = cairo_image_surface_get_height(surface);
> +        break;
> +#if ENABLE(ACCELERATED_2D_CANVAS)
> +    case CAIRO_SURFACE_TYPE_GL:
> +        width = cairo_gl_surface_get_width(surface);
> +        height = cairo_gl_surface_get_height(surface);
> +        break;
> +#endif
> +    default:
> +        ASSERT_NOT_REACHED();
> +        break;
> +    }
> +    return IntSize(width, height);

This can just be:

    switch (cairo_surface_get_type(surface)) {
    case CAIRO_SURFACE_TYPE_IMAGE:
        return IntSize(cairo_image_surface_get_width(surface), cairo_image_surface_get_height(surface));
#if ENABLE(ACCELERATED_2D_CANVAS)
    case CAIRO_SURFACE_TYPE_GL:
        return IntSize(cairo_gl_surface_get_width(surface), cairo_gl_surface_get_height(surface));
#endif
    default:
        ASSERT_NOT_REACHED();
        return IntSize();
    }

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:225
> +        RefPtr<cairo_surface_t> tmpSurface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, m_imageWidth, m_imageHeight));
> +        RefPtr<cairo_t> cr = adoptRef(cairo_create(tmpSurface.get()));
> +        cairo_set_source_surface(cr.get(), m_imageSurface.get(), 0, 0);
> +        cairo_paint(cr.get());
> +        m_imageSurface = tmpSurface;

Could you use copyRectFromOneSurfaceToAnother? Don't forget to use the source operator.

> Source/WebCore/platform/graphics/gtk/ImageBufferGtk.cpp:53
> +        IntSize size = cairoSurfaceSize(surface);
> +        RefPtr<cairo_surface_t> newSurface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_RGB24, size.width(), size.height()));
> +        RefPtr<cairo_t> cr = adoptRef(cairo_create(newSurface.get()));
> +        cairo_set_source_surface(cr.get(), surface, 0, 0);
> +        cairo_paint(cr.get());
> +        pixbuf = adoptGRef(cairoSurfaceToGdkPixbuf(newSurface.get()));

This is a lot less efficient if the surface is already an image.
Comment 16 arno. 2013-08-12 12:35:31 PDT
Created attachment 208554 [details]
updated patch
Comment 17 Chris Dumez 2013-08-12 13:31:35 PDT
Comment on attachment 208554 [details]
updated patch

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

> Source/WebCore/platform/graphics/cairo/BitmapImageCairo.cpp:56
> +    m_size = cairoSurfaceSize(nativeImage.get());

nit: looks like this can be moved to the init list.

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:252
> +        break;

nit: Useless break after a return.

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:256
> +        break;

ditto.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:223
> +        m_imageSurface = tmpSurface;

nit: you can release() tmpSurface here
Comment 18 kov's GTK+ EWS bot 2013-08-12 14:15:27 PDT
Comment on attachment 208554 [details]
updated patch

Attachment 208554 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1452202
Comment 19 Build Bot 2013-08-23 14:04:09 PDT
Comment on attachment 208554 [details]
updated patch

Attachment 208554 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1526762
Comment 20 arno. 2013-08-27 15:09:20 PDT
Created attachment 209807 [details]
updated patch

build failure have possibly been fixed by bug #118621
Comment 21 Martin Robinson 2013-08-27 15:28:49 PDT
Comment on attachment 209807 [details]
updated patch

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

Looks pretty good to, just have a few small suggestions...

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DCairo.cpp:222
> +    // if m_imageSurface is not an image, extract a copy of the surface
> +    if (cairo_surface_get_type(m_imageSurface.get()) != CAIRO_SURFACE_TYPE_IMAGE) {
> +        RefPtr<cairo_surface_t> tmpSurface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, m_imageWidth, m_imageHeight));
> +        copyRectFromOneSurfaceToAnother(m_imageSurface.get(), tmpSurface.get(), IntSize(), IntRect(0, 0, m_imageWidth, m_imageHeight), IntSize(), CAIRO_OPERATOR_SOURCE);
> +        m_imageSurface = tmpSurface.release();
> +    }
> +

It seems this can only happen in the else case above, so perhaps this conversion should happen there, so that below we can ASSERT that the surface type is image.

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:29
> +#include "CairoUtilities.h"
> +
>  #include "GdkCairoUtilities.h"

Extra newline here.

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:33
> +#include "IntSize.h"
>  
>  #include <cairo.h>

And here.

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:41
> +    return gdk_pixbuf_get_from_surface(surface, 0, 0, size.width(), size.height());

Are we sure that gdk_pixbuf_get_from_surface can handle non-image surfaces?
Comment 22 arno. 2013-08-27 18:03:10 PDT
(In reply to comment #21)
> (From update of attachment 209807 [details])

> > Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:29
> > +#include "CairoUtilities.h"
> > +
> >  #include "GdkCairoUtilities.h"
> 
> Extra newline here.

I get a warning by the style checker if I remove it
 You should add a blank line after implementation file's own header.  [build/include_order] [4]



> > Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:41
> > +    return gdk_pixbuf_get_from_surface(surface, 0, 0, size.width(), size.height());
> 
> Are we sure that gdk_pixbuf_get_from_surface can handle non-image surfaces?

yes, gdk_pixbuf_get_from_surface converts internally non image surface to image ones
Comment 23 Martin Robinson 2013-08-27 18:04:11 PDT
(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 209807 [details] [details])
> 
> > > Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:29
> > > +#include "CairoUtilities.h"
> > > +
> > >  #include "GdkCairoUtilities.h"
> > 
> > Extra newline here.
> 
> I get a warning by the style checker if I remove it
>  You should add a blank line after implementation file's own header.  [build/include_order] [4]

Oops. This newline is correct. The next one looks extraneous though. :)
Comment 24 arno. 2013-08-27 18:08:31 PDT
Created attachment 209829 [details]
updated patch
Comment 25 WebKit Commit Bot 2013-08-29 10:22:27 PDT
Comment on attachment 209829 [details]
updated patch

Clearing flags on attachment: 209829

Committed r154819: <http://trac.webkit.org/changeset/154819>
Comment 26 WebKit Commit Bot 2013-08-29 10:22:32 PDT
All reviewed patches have been landed.  Closing bug.