Bug 118808 – [cairo] canvas drawing on itself doesn't work with accelerated canvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[arno.]

Hi, when drawing a canvas on itself with accelerated canvas, nothing is drawn
See test url for an example

[arno.]

Created attachment 206931 [details]
patch proposal

[EFL EWS Bot]

Comment on attachment 206931 [details]
patch proposal

Attachment 206931 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1100316

[EFL EWS Bot]

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

[arno.]

Created attachment 207013 [details]
updated patch: guard with ENABLE(ACCELERATED_2D_CANVAS) when needed

[Chris Dumez]

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?

[arno.]

(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

[Martin Robinson]

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

[arno.]

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.

[arno.]

Created attachment 207293 [details]
updated patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[arno.]

Created attachment 207969 [details]
updated patch: extractImage still didn't work when getting data from an accelerated canvas

[Martin Robinson]

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.

[arno.]

Created attachment 208554 [details]
updated patch

[Chris Dumez]

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

[kov's GTK+ EWS bot]

Comment on attachment 208554 [details]
updated patch

Attachment 208554 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1452202

[Build Bot]

Comment on attachment 208554 [details]
updated patch

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

[arno.]

Created attachment 209807 [details]
updated patch

build failure have possibly been fixed by bug #118621

[Martin Robinson]

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?

[arno.]

(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

[Martin Robinson]

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

[arno.]

Created attachment 209829 [details]
updated patch

[WebKit Commit Bot]

Comment on attachment 209829 [details]
updated patch

Clearing flags on attachment: 209829

Committed r154819: <http://trac.webkit.org/changeset/154819>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.