Bug 160804

Summary: ctx.drawImage should clip source rect if it is outside the source image
Product: WebKit Reporter: Rich Harris <richard.a.harris>
Component: CanvasAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Major CC: cdumez, commit-queue, darin, dino, esprehn+autocc, gyuyoung.kim, sabouhallawa, simon.fraser
Priority: P2    
Version: Safari Technology Preview   
Hardware: Mac   
OS: OS X 10.11   
URL: https://html.spec.whatwg.org/multipage/scripting.html#dom-context-2d-drawimage
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews123 for ios-simulator-elcapitan-wk2
none
Patch
none
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2
none
Patch none

Description Rich Harris 2016-08-12 08:49:46 PDT
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.
Comment 1 Chris Dumez 2016-08-15 09:02:37 PDT
I can reproduce on ToT. I'll investigate.
Comment 2 Chris Dumez 2016-08-15 09:55:24 PDT
Not a JS bindings bug. We fail the following check in the drawImage() implementation:
    if (!imageRect.contains(normalizedSrcRect))
        return;
Comment 3 Chris Dumez 2016-08-15 10:01:12 PDT
(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.
Comment 4 Chris Dumez 2016-08-15 10:03:06 PDT
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.
Comment 5 Chris Dumez 2016-08-15 11:08:38 PDT
Created attachment 286068 [details]
Patch
Comment 6 Said Abou-Hallawa 2016-08-15 11:19:08 PDT
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()
Comment 7 Chris Dumez 2016-08-15 11:46:40 PDT
Created attachment 286071 [details]
Patch
Comment 8 Chris Dumez 2016-08-15 11:52:08 PDT
(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 :)
Comment 9 Build Bot 2016-08-15 12:44:24 PDT
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
Comment 10 Build Bot 2016-08-15 12:44:27 PDT
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
Comment 11 Chris Dumez 2016-08-15 13:28:04 PDT
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.
Comment 12 Chris Dumez 2016-08-15 15:29:32 PDT
Created attachment 286097 [details]
Patch
Comment 13 Build Bot 2016-08-15 17:10:40 PDT
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
Comment 14 Build Bot 2016-08-15 17:10:44 PDT
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
Comment 15 Chris Dumez 2016-08-15 19:26:34 PDT
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.
Comment 16 Chris Dumez 2016-08-15 19:29:14 PDT
Created attachment 286136 [details]
Patch
Comment 17 Chris Dumez 2016-08-15 19:29:45 PDT
Now using Canvas in the expected result as well, hoping this will make the iOS bot happy.
Comment 18 Darin Adler 2016-08-15 23:09:09 PDT
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.
Comment 19 Chris Dumez 2016-08-16 09:36:39 PDT
(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.
Comment 20 Chris Dumez 2016-08-16 12:10:34 PDT
Comment on attachment 286136 [details]
Patch

Clearing flags on attachment: 286136

Committed r204517: <http://trac.webkit.org/changeset/204517>
Comment 21 Chris Dumez 2016-08-16 12:10:40 PDT
All reviewed patches have been landed.  Closing bug.