Bug 160804 – ctx.drawImage should clip source rect if it is outside the source image

• Component: Canvas
• Status: RESOLVED FIXED

[Rich Harris]

Overview: 

Using ctx.drawImage(image, sx, sy, swidth, sheight, dx, dy, dwidth, dheight), where ctx is a CanvasRenderingContext2D, fails if any of sx, sy, swidth or sheight are non-integers. No error is given.

Steps to Reproduce:

Create an HTML page with the following code, and open it in Safari or Safari Technology Preview:

<canvas id='target' width='100' height='100'/>

<script>
  image = new Image();

  image.onload = function () {
    // Changing 0.1 to 0 will cause the image to appear
    target.getContext( '2d' ).drawImage( image, 0.1, 0, 100, 100, 0, 0, 100, 100 );
  };

  image.src = 'http://placehold.it/100x100';
</script>

Actual Results:

The canvas remains blank.

Expected Results:

The image is drawn to the canvas (albeit possibly with some sampling artifacts).

Build Date & Platform: Date and platform of the build in which you first encountered the bug.

Safari Version 9.1.2 (11601.7.7) on Mac OS 10.

Additional Information:

This works as expected in all other browsers. The dx, dy, dwidth and dheight arguments work whether or not they are integers.

[Chris Dumez]

I can reproduce on ToT. I'll investigate.

[Chris Dumez]

Not a JS bindings bug. We fail the following check in the drawImage() implementation:
    if (!imageRect.contains(normalizedSrcRect))
        return;

[Chris Dumez]

(In reply to comment #2)
> Not a JS bindings bug. We fail the following check in the drawImage()
> implementation:
>     if (!imageRect.contains(normalizedSrcRect))
>         return;

srcRect:
[0.100000, 0.000000, 100.000000 100.000000]
normalizedSrcRect:
[0.100000, 0.000000, 100.000000 100.000000]
imageRect:
[0.000000, 0.000000, 100.000000 100.000000]

Technically, it is true that imageRect.contains does not fully contain normalizedSrcRect / srcRect.

[Chris Dumez]

In this case, the spec says:
"When the source rectangle is outside the source image, the source rectangle must be clipped to the source image and the destination rectangle must be clipped in the same proportion."

However, WebKit ignores the call instead of clipping it.

[Chris Dumez]

Created attachment 286068 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 286068 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1403
> +        normalizedDstRect.setWidth(normalizedDstRect.width() - originalNormalizedSrcRect.width() + normalizedSrcRect.width());
> +        normalizedDstRect.setHeight(normalizedDstRect.height() - originalNormalizedSrcRect.height() + normalizedSrcRect.height());

From the statement "When the source rectangle is outside the source image, the source rectangle  must be clipped to the source image and the destination rectangle must be  clipped in the same proportion." I would expect these two lines to be 

normalizedDstRect.setWidth(normalizedDstRect.width() * normalizedSrcRect.width() / originalNormalizedSrcRect.width());
normalizedDstRect.setHeight(normalizedDstRect.height() * normalizedSrcRect.height() * originalNormalizedSrcRect.height());

I think the goal is ensure that ratio originalNormalizedDstRect.size() / originalNormalizedSrcRect.size() is equal to the ratio normalizedDstRect.size() / normalizedSrcRect.size()

[Chris Dumez]

Created attachment 286071 [details]
Patch

[Chris Dumez]

(In reply to comment #6)
> Comment on attachment 286068 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286068&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1403
> > +        normalizedDstRect.setWidth(normalizedDstRect.width() - originalNormalizedSrcRect.width() + normalizedSrcRect.width());
> > +        normalizedDstRect.setHeight(normalizedDstRect.height() - originalNormalizedSrcRect.height() + normalizedSrcRect.height());
> 
> From the statement "When the source rectangle is outside the source image,
> the source rectangle  must be clipped to the source image and the
> destination rectangle must be  clipped in the same proportion." I would
> expect these two lines to be 
> 
> normalizedDstRect.setWidth(normalizedDstRect.width() *
> normalizedSrcRect.width() / originalNormalizedSrcRect.width());
> normalizedDstRect.setHeight(normalizedDstRect.height() *
> normalizedSrcRect.height() * originalNormalizedSrcRect.height());
> 
> I think the goal is ensure that ratio originalNormalizedDstRect.size() /
> originalNormalizedSrcRect.size() is equal to the ratio
> normalizedDstRect.size() / normalizedSrcRect.size()

You make a good point :)

[Build Bot]

Comment on attachment 286071 [details]
Patch

Attachment 286071 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1876646

New failing tests:
fast/canvas/drawImage-srcRect-clipping.html

[Build Bot]

Created attachment 286077 [details]
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Chris Dumez]

There is barely any visible difference between actual / expected on the iOS-simulator bot. I am not sure how to resolve except by skipping the test on this platform. Please advise.

[Chris Dumez]

Created attachment 286097 [details]
Patch

[Build Bot]

Comment on attachment 286097 [details]
Patch

Attachment 286097 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1877938

New failing tests:
fast/canvas/drawImage-srcRect-clipping.html

[Build Bot]

Created attachment 286121 [details]
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Chris Dumez]

So what it looks like on iOS is that the green part of the canvas is blurry along the bottom and the right side. However, it is sharp in the reference html. This difference is apparently enough.

[Chris Dumez]

Created attachment 286136 [details]
Patch

[Chris Dumez]

Now using Canvas in the expected result as well, hoping this will make the iOS bot happy.

[Darin Adler]

Comment on attachment 286136 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1403
> +        normalizedDstRect.setWidth(normalizedDstRect.width() * normalizedSrcRect.width() / originalNormalizedSrcRect.width());
> +        normalizedDstRect.setHeight(normalizedDstRect.height() * normalizedSrcRect.height() / originalNormalizedSrcRect.height());

Does not seem to correctly handle the case where the source rectangle had a negative X or Y coordinate.

[Chris Dumez]

(In reply to comment #18)
> Comment on attachment 286136 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286136&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1403
> > +        normalizedDstRect.setWidth(normalizedDstRect.width() * normalizedSrcRect.width() / originalNormalizedSrcRect.width());
> > +        normalizedDstRect.setHeight(normalizedDstRect.height() * normalizedSrcRect.height() / originalNormalizedSrcRect.height());
> 
> Does not seem to correctly handle the case where the source rectangle had a
> negative X or Y coordinate.

width / height will always be positive here since the srcRect has been normalized already.

[Chris Dumez]

Comment on attachment 286136 [details]
Patch

Clearing flags on attachment: 286136

Committed r204517: <http://trac.webkit.org/changeset/204517>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.