Bug 194746 - drawImage() clears the canvas if it's the source of the image and globalCompositeOperation is "copy"
Summary: drawImage() clears the canvas if it's the source of the image and globalCompo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: Safari Technology Preview
Hardware: Macintosh macOS 10.14
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-16 08:10 PST by susisu2413
Modified: 2019-02-20 14:28 PST (History)
11 users (show)

See Also:


Attachments
Patch (6.34 KB, patch)
2019-02-19 15:30 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (6.97 KB, patch)
2019-02-20 12:04 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description susisu2413 2019-02-16 08:10:17 PST
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.
Comment 1 Alexey Proskuryakov 2019-02-18 08:46:48 PST
Thank you for the report! Do you happen to know if this ever worked before in Safari?
Comment 2 Said Abou-Hallawa 2019-02-18 09:28:44 PST
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.
Comment 3 Simon Fraser (smfr) 2019-02-18 10:45:21 PST
So we need to read out the source before clearing in this case?
Comment 4 Radar WebKit Bug Importer 2019-02-18 10:46:24 PST
<rdar://problem/48169601>
Comment 5 susisu2413 2019-02-18 17:12:36 PST
(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.
Comment 6 Said Abou-Hallawa 2019-02-19 15:30:41 PST
Created attachment 362445 [details]
Patch
Comment 7 Simon Fraser (smfr) 2019-02-19 15:35:22 PST
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?
Comment 8 Said Abou-Hallawa 2019-02-19 16:29:23 PST
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))
Comment 9 Dean Jackson 2019-02-19 17:31:52 PST
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.
Comment 10 Dean Jackson 2019-02-19 17:32:26 PST
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.
Comment 11 Said Abou-Hallawa 2019-02-20 12:04:37 PST
Created attachment 362522 [details]
Patch
Comment 12 Said Abou-Hallawa 2019-02-20 12:10:46 PST
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.
Comment 13 WebKit Commit Bot 2019-02-20 14:28:19 PST
Comment on attachment 362522 [details]
Patch

Clearing flags on attachment: 362522

Committed r241840: <https://trac.webkit.org/changeset/241840>
Comment 14 WebKit Commit Bot 2019-02-20 14:28:21 PST
All reviewed patches have been landed.  Closing bug.