Bug 194746 – drawImage() clears the canvas if it's the source of the image and globalCompositeOperation is "copy"

• Component: Canvas
• Status: RESOLVED FIXED

When copying a region of a canvas to another region of the same canvas, drawImage does not work correctly if those regions have overlap.

A reproduction code is here: https://jsfiddle.net/kfcea804/
  Expected: Two green overlapping rectangles are drawn.
  Actual: A part of one rectangle is chipped (drawn as white, not green).

The chipped region matches the area of the source region that is overwritten by the drawImage.
I'm not sure, but the cause of this issue may be that WebKit does not taking a copy of the source image before the drawing operation, as opposed to what HTML Living Standard says:

> When a canvas element is drawn onto itself, the drawing model requires the source to be copied before the image is drawn, so it is possible to copy parts of a canvas element onto overlapping parts of itself.

https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-drawimage

I am using Safari TP Release 75 (Safari 12.2, WebKit 14608.1.3.0.2) on macOS 10.14.3.
I also checked that this issue is also reproduced with WebKit r241645.

[Alexey Proskuryakov]

Thank you for the report! Do you happen to know if this ever worked before in Safari?

[Said Abou-Hallawa]

Looking at the test case:

The bug happens because of this setting:
    ctx.globalCompositeOperation = "copy";

Looking at the source code of CanvasRenderingContext2DBase::drawImage() and when 

if (state().globalComposite == CompositeCopy) {
    clearCanvas();
    c->drawImageBuffer(*buffer, dstRect, srcRect, ImagePaintingOptions(state().globalComposite, state().globalBlend));
    didDrawEntireCanvas();
}

Calling clearCanvas() will clear the entire canvas rectangle. Because the test case clips to the destination rect(64, 64, 128, 128), clearCanvas() will only clear this rectangle. But this destination rectangle overlaps the source rectangle rect(0, 0, 128, 128). So the intersection rectangle (64, 64, 128, 128) is also cleared from the source rectangle. And this is why a white rectangle in the middle of the cnavas is drawn.

[Simon Fraser (smfr)]

So we need to read out the source before clearing in this case?

[Radar WebKit Bug Importer]

<rdar://problem/48169601>

(In reply to Alexey Proskuryakov from comment #1)
> Thank you for the report! Do you happen to know if this ever worked before
> in Safari?

No, I don't know it worked before.

[Said Abou-Hallawa]

Created attachment 362445 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 362445 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1665
> +        if (&sourceCanvas == &canvasBase()) {
> +            auto imageBuffer = drawCanvasToBuffer(srcRect);
> +            clearCanvas();
> +            c->drawImageBuffer(*imageBuffer, dstRect, { { }, srcRect.size() }, ImagePaintingOptions(state().globalComposite, state().globalBlend));

Should we look to see if srcRect and dstRect overlap and only do the copy in that case?

[Said Abou-Hallawa]

Comment on attachment 362445 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1665
>> +            c->drawImageBuffer(*imageBuffer, dstRect, { { }, srcRect.size() }, ImagePaintingOptions(state().globalComposite, state().globalBlend));
> 
> Should we look to see if srcRect and dstRect overlap and only do the copy in that case?

No. Because if srcRect and dstRect do not overlap, we will fall through the else-statement which will clearCanvas() before drawImageBuffer() .

For example, the new test layout in this patch will show nothing if I change the above if statement to be:
 
if (&sourceCanvas == &canvasBase() && srcRect.intersects(dstRect))

[Dean Jackson]

Comment on attachment 362445 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1602
> +std::unique_ptr<ImageBuffer> CanvasRenderingContext2DBase::drawCanvasToBuffer(const FloatRect& srcRect) const

I wonder if this should be called copyCanvasRegionToBuffer?

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1611
> +    auto& canvas = downcast<HTMLCanvasElement>(canvasBase());
> +    ImageBuffer* srcBuffer = canvas.buffer();
> +    if (!srcBuffer)
> +        return nullptr;

This can go first in the function to skip creating the dstBuffer if it fails.

[Dean Jackson]

Comment on attachment 362445 [details]
Patch

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

> LayoutTests/fast/canvas/canvas-drawImage-composite-copy.html:2
> +    <canvas id="canvas" width="200" height="200"></canvas>

We should probably make this a WPT and submit it back.

[Said Abou-Hallawa]

Created attachment 362522 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 362445 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1602
>> +std::unique_ptr<ImageBuffer> CanvasRenderingContext2DBase::drawCanvasToBuffer(const FloatRect& srcRect) const
> 
> I wonder if this should be called copyCanvasRegionToBuffer?

I moved this function to ImageBuffer and I name it copyRectToBuffer(). So please have a look at the new patch.

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:1611
>> +        return nullptr;
> 
> This can go first in the function to skip creating the dstBuffer if it fails.

You are right. After moving this function to ImageBuffer, I call it from CanvasRenderingContext2DBase::drawImage() from buffer->copyRectToBuffer(). We check nullability of buffer before this call. So this check is not needed anymore.

>> LayoutTests/fast/canvas/canvas-drawImage-composite-copy.html:2
>> +    <canvas id="canvas" width="200" height="200"></canvas>
> 
> We should probably make this a WPT and submit it back.

I will talk to Youenn Fablet about the process to get it as WPT.

[WebKit Commit Bot]

Comment on attachment 362522 [details]
Patch

Clearing flags on attachment: 362522

Committed r241840: <https://trac.webkit.org/changeset/241840>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.