Bug 118621 – [cairo] memory corruption with putImageData and accelerated canvas.

• Component: Canvas
• Status: RESOLVED FIXED

[arno.]

Hi,
when calling putImageData with non 0 dx/dy parameters, I observe a memory corruption if accelerated canvas is enabled for the underlying canvas.
Launch http://renevier.net/tmp/canvas_malloc_assert.html with MiniBrowser to reproduce (make sure accelerated canvas is enabled; --enable-accelerated-2d-canvas).
I get following error on stdout:

*** glibc detected *** /home/arno/webkit/WebKit/WebKitBuild/Release/Programs/.libs/lt-WebKitWebProcess: malloc(): memory corruption: 0x08e42f00 ***

[arno.]

Created attachment 206697 [details]
patch proposal

in this patch, we use access data directly if we have an image surface. Otherwise, we create image surface and copy data from (or to) it. This is already the current underlying mechanism, but it's made more explicit. Also, we create the temporary image surface with the minimal needed size. This reduces data transfer

[Martin Robinson]

Comment on attachment 206697 [details]
patch proposal

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

I don't think the bug or the patch explains what the original problem was.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:245
> +
> +    // If we already have an image, we read directly the underlying data;
> +    // otherwise we create a temporary image surface, and copy data surface
> +    // into it first
> +    if (surfaceType == CAIRO_SURFACE_TYPE_IMAGE) {
> +        imageSurface = data.m_surface.get();
> +        dx = originx;
> +        dy = originy;
> +    } else {
> +        IntRect area = intersection(rect, IntRect(0, 0, size.width(), size.height()));
> +        imageSurface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, area.width(), area.height()));
> +        copySurface(data.m_surface.get(), imageSurface.get(), area.size(), area.location(), IntPoint(0, 0));
> +        dx = 0;
> +        dy = 0;
> +    }
> +
> +    unsigned char* dataSrc = cairo_image_surface_get_data(imageSurface.get());
> +    unsigned char* dataDst = result->data();
> +

If we are reading the entire image, then map_to_image can be faster since it can optimize the operation via PBOs. It doesn't do that yet, but it should in the future.

[arno.]

Created attachment 206924 [details]
new patch proposal

[Martin Robinson]

Comment on attachment 206924 [details]
new patch proposal

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

Thanks for fixing this. I've just a couple nitpicky things.

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:248
> +void paintSurfaceToAnother(cairo_surface_t* from, cairo_surface_t* to, const IntSize& sourceOffset, const IntSize& destOffset, const IntRect& rect)
> +{
> +    RefPtr<cairo_t> context = adoptRef(cairo_create(to));
> +    cairo_translate(context.get(), destOffset.width(), destOffset.height());
> +    cairo_set_operator(context.get(), CAIRO_OPERATOR_SOURCE);
> +    copyRectFromCairoSurfaceToContext(from, context.get(), sourceOffset, rect);
> +}
> +

It seems you could write this in terms of copyRectFromOneSurfaceToAnother by adding a few more parameters with default values.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:183
> +PassRefPtr<cairo_surface_t> imageSurfaceFrom(cairo_surface_t* surface, const IntRect& rect, IntPoint* outOffset)

outOffset can be a reference and then you don't have to worry about getting null. I think I'd prefer a name like mapSurfaceToImageAndAdjustRect. You could pass in a non-const rect and just adjust it.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:197
> +    // If we already have an image, we return directly the surface; otherwise
> +    // we create new surface image
> +    cairo_surface_type_t surfaceType = cairo_surface_get_type(surface);
> +
> +    RefPtr<cairo_surface_t> imageSurface;
> +
> +    // If we already have an image, we write directly to the underlying data;
> +    // otherwise we create a temporary surface image
> +    if (surfaceType == CAIRO_SURFACE_TYPE_IMAGE) {
> +        imageSurface = surface;
> +        outOffset->setX(rect.x());
> +        outOffset->setY(rect.y());
> +    } else {

Maybe just use an early return?

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:236
> +    IntPoint delta;

delta -> offsetInImage?

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:239
> +    int dx = delta.x();
> +    int dy = delta.y();

I don't think you need these temporaries. Why not just modify originX and originY?

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:327
> +    IntPoint delta;
> +    RefPtr<cairo_surface_t> imageSurface = imageSurfaceFrom(m_data.m_surface.get(), IntRect(destx, desty, numColumns, numRows), &delta);
> +    int dx = delta.x();
> +    int dy = delta.y();
> +

Ditto.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:328
> +    unsigned char* dataDst = cairo_image_surface_get_data(imageSurface.get());

Let's use a better name since you are changing this line. How about pixelData?

[arno.]

(In reply to comment #4)
> (From update of attachment 206924 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206924&action=review

> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:239
> > +    int dx = delta.x();
> > +    int dy = delta.y();
> 
> I don't think you need these temporaries. Why not just modify originX and originY?

ok

> > Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:327
> > +    IntPoint delta;
> > +    RefPtr<cairo_surface_t> imageSurface = imageSurfaceFrom(m_data.m_surface.get(), IntRect(destx, desty, numColumns, numRows), &delta);
> > +    int dx = delta.x();
> > +    int dy = delta.y();
> > +
> 
> Ditto.

In this case, we need the original destx/desty to paint temporary image back into original surface

[arno.]

Created attachment 207131 [details]
new patch proposal

[Martin Robinson]

Comment on attachment 207131 [details]
new patch proposal

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

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

This isn't entirely accurate. You should probably say that accelerated canvas is not enabled for testing yet.

> Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp:235
> +void copyRectFromOneSurfaceToAnother(cairo_surface_t* from, cairo_surface_t* to, const IntSize& sourceOffset, const IntRect& rect, const IntSize& destOffset, cairo_operator_t cairo_op)

cairo_op is not a WebKit style name. How about operator or cairoOperator?

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:183
> +PassRefPtr<cairo_surface_t> mapSurfaceToImageAndAdjustRect(cairo_surface_t* surface, const IntRect& rect, IntPoint& outOffset)

As I said before, I still think it's a good idea to make rect and in/out parameter and dispense with outOffset altogether.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:197
> +        imageSurface = surface;
> +        outOffset.setX(rect.x());
> +        outOffset.setY(rect.y());
> +        return imageSurface.release();

I think you can dispense with the imageSurface variable entirely and just return the pointers. They should be properly converted into PassRefPtr.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:203
> +    imageSurface = adoptRef(cairo_image_surface_create(CAIRO_FORMAT_ARGB32, rect.width(), rect.height()));
> +    outOffset.setX(0);
> +    outOffset.setY(0);
> +    return imageSurface.release();

...except here where you should return adoptRef(...).

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:239
> -    int stride = cairo_image_surface_get_stride(imageSurface);
> +    IntPoint offsetInImage;
> +    RefPtr<cairo_surface_t> imageSurface = mapSurfaceToImageAndAdjustRect(data.m_surface.get(), IntRect(originx, originy, numColumns, numRows), offsetInImage);
> +    originx = offsetInImage.x();
> +    originy = offsetInImage.y();

Instead of using originx and originy, please just use an IntRect to store all the properties of the sourceRect.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:247
> +

Extra newline here.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:328
> +    IntPoint offsetInImage;
> +    RefPtr<cairo_surface_t> imageSurface = mapSurfaceToImageAndAdjustRect(m_data.m_surface.get(), IntRect(destx, desty, numColumns, numRows), offsetInImage);
> +    int dx = offsetInImage.x();
> +    int dy = offsetInImage.y();
> +
> +    unsigned char* pixelData = cairo_image_surface_get_data(imageSurface.get());

I don't see the need for temporaries here. Can't you just use a sourceRect and use sourceRect.x() and sourceRect.y() to paint back?

[arno.]

(In reply to comment #7)
> (From update of attachment 207131 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207131&action=review

> Instead of using originx and originy, please just use an IntRect to store all the properties of the sourceRect.

Do you mean accessing rect.x() / rect.y() in the hot loop ? If so, I fear there would be performance issues. We already avoid Color::colorFromPremultipliedARGB because this function call was deemed too expensive.

[arno.]

Created attachment 207356 [details]
updated patch

[Martin Robinson]

Comment on attachment 207356 [details]
updated patch

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

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:190
> +PassRefPtr<cairo_surface_t> mapSurfaceToImageAndAdjustRect(cairo_surface_t* surface, IntRect& rect)

This should be renamed from map to copy since we are no longer trying to replicate the Cairo map/ummap API.

> Source/WebCore/platform/graphics/cairo/ImageBufferCairo.cpp:240
> +        copyRectFromOneSurfaceToAnother(data.m_surface.get(), imageSurface.get(), IntSize(-area.x(), -area.y()), IntRect(IntPoint::zero(), area.size()), IntSize(), CAIRO_OPERATOR_SOURCE);

You can just use IntPoint() here instead of IntPoint::zero()

[arno.]

Created attachment 208560 [details]
updated patch

[arno.]

Created attachment 208561 [details]
updated patch with Reviewed By field filled

[WebKit Commit Bot]

Comment on attachment 208561 [details]
updated patch with Reviewed By field filled

Clearing flags on attachment: 208561

Committed r153961: <http://trac.webkit.org/changeset/153961>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.