Bug 67652 – Negative width/height values not always treated properly by 2D canvas drawImage

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Canvas

[Justin Novosad]

Created attachment 106438 [details]
Test that demonstrates this bug, html part

The 2d canvas drawImage method handles negative source rectangle dimension correctly when the source image is another canvas.  When the source is an image object or a video object, rendering fails.  The correct behavior, according to the specification, is to take the rectangle bounded by (sx, sy) and (sx+sw, sy+sh). Pixel ordering should _NOT_ be flipped when sw or sh are negative.  The current layout tests for this use case only cover canvas to canvas draws (the only path that works correctly).  So a new layout test is necessary.

Attached is a tentative layout test that covers the image to canvas draw path. The test should be placed in LayoutTests/fast/canvas

[Justin Novosad]

Created attachment 106439 [details]
Test that demonstrates this bug, js part
Note: Until the fix for 65709 is submitted, this test may not work. Test also covers bugs 66574, so not all the reported failures are related to this bug

[Mark Hahnenberg]

*** Bug 67626 has been marked as a duplicate of this bug. ***

[Justin Novosad]

(In reply to comment #2)
> *** Bug 67626 has been marked as a duplicate of this bug. ***

Ignore. This was a typo

[Rashmi Shyamasundar]

I would like to submit a patch for this issue. I see that this issue is assigned to Mr. Justin Novosad. But, there is no update since "2011-09-07". 

Hoping that it is okay with everyone if I submit a patch for this issue.

[Justin Novosad]

It's all yours.

[Rashmi Shyamasundar]

In short, the issue is that -
Whenever the dimensions specified for the source rectangle is outside the source image, the canvas-drawImage fails. It is failing in the cases of both, canvas to canvas draw and image to canvas draw.

[Rashmi Shyamasundar]

Dear Mr. Martin Robinson,

Below are the few lines which I have extracted from the canvas spec - http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-path-addtext .

"The source rectangle is the rectangle whose corners are the four points (sx, sy), (sx+sw, sy), (sx+sw, sy+sh), (sx, sy+sh).

The destination rectangle is the rectangle whose corners are the four points (dx, dy), (dx+dw, dy), (dx+dw, dy+dh), (dx, dy+dh).

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."

Currently, webkit does not conform to this spec.

The patch I am uploading now, shows the approach which we can take to conform to the above spec. If the approach looks fine, please let me know. I will thoroughly test this patch and write the required layout tests.

Thanks,
Rashmi

[Rashmi Shyamasundar]

Created attachment 205262 [details]
Patch

[Rashmi Shyamasundar]

Created attachment 205366 [details]
scenarios.png

The attached image - "scenarios.png" shows the different cases or scenarios which should be considered for implementing the spec mentioned in this bug description.

[Rashmi Shyamasundar]

Created attachment 205394 [details]
Patch

[Rashmi Shyamasundar]

https://bugs.webkit.org/attachment.cgi?id=205394&action=review . This is the complete patch, with the required Layout-test. Please review.

[Rashmi Shyamasundar]

Created attachment 205451 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 205451 [details]
Patch

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

I think you need to check if there is regression on whole layout test.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:92
> +static bool clipRectangles(FloatRect &imageRect, FloatRect &normalizedSrcRect, FloatRect &normalizedDstRect)

Wrong & place.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:104
> +        float clipRatio = (normalizedSrcRect.width() - (imageRect.x() + imageRect.width() - normalizedSrcRect.x()) )/ normalizedSrcRect.width();

Missing a space in )/ normalizedSrcRect.width();

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:120
> +        float clipRatio = (normalizedSrcRect.height() - (imageRect.y() + imageRect.height() - normalizedSrcRect.y()) )/ normalizedSrcRect.height();

ditto.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1339
> +    if (!imageRect.contains(normalizedSrcRect)) 

Remove indentation after ).

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1340
> +        if (!clipRectangles(imageRect, normalizedSrcRect, normalizedDstRect))

Why can't we use below ?

if (!videoRect.contains(normalizedSrcRect) && !clipRectangles(videoRect, normalizedSrcRect, normalizedDstRect))
    return;

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1418
> +        if (!clipRectangles(srcCanvasRect, normalizedSrcRect, normalizedDstRect))

ditto.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1516
> +        if (!clipRectangles(videoRect, normalizedSrcRect, normalizedDstRect))

ditto ?

[Rashmi Shyamasundar]

Created attachment 205773 [details]
Patch

[Rashmi Shyamasundar]

With this patch, I ran all the 2d-canvas layouttests under LayoutTests/fast/canvas and LayoutTests/canvas/philip/tests. There is no regression.

[Dirk Schulze]

Comment on attachment 205773 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:120
> +static bool clipRectangles(FloatRect& imageRect, FloatRect& normalizedSrcRect, FloatRect& normalizedDstRect)
> +{
> +    if (normalizedSrcRect.x() < imageRect.x()) {
> +        float clipRatio = (imageRect.x() - normalizedSrcRect.x()) / normalizedSrcRect.width();
> +        if (clipRatio >= 1)
> +            return false;
> +        normalizedDstRect.setX(normalizedDstRect.x() + (clipRatio * normalizedDstRect.width()));
> +        normalizedDstRect.setWidth(normalizedDstRect.width() - (clipRatio * normalizedDstRect.width()));
> +        normalizedSrcRect.setWidth(normalizedSrcRect.width() - (clipRatio * normalizedSrcRect.width()));
> +        normalizedSrcRect.setX(imageRect.x());
> +    }
> +    if ((normalizedSrcRect.x() + normalizedSrcRect.width())  > (imageRect.x() + imageRect.width())) {
> +        float clipRatio = (normalizedSrcRect.width() - (imageRect.x() + imageRect.width() - normalizedSrcRect.x())) / normalizedSrcRect.width();
> +        if (clipRatio >= 1)
> +            return false;
> +        normalizedDstRect.setWidth(normalizedDstRect.width() - (clipRatio * normalizedDstRect.width()));
> +        normalizedSrcRect.setWidth(normalizedSrcRect.width() - (clipRatio * normalizedSrcRect.width()));
> +    }
> +    if (normalizedSrcRect.y() < imageRect.y()) {
> +        float clipRatio = (imageRect.y() - normalizedSrcRect.y()) / normalizedSrcRect.height();
> +        if (clipRatio >= 1)
> +            return false;
> +        normalizedDstRect.setY(normalizedDstRect.y() + (clipRatio * normalizedDstRect.height()));
> +        normalizedDstRect.setHeight(normalizedDstRect.height() - (clipRatio * normalizedDstRect.height()));
> +        normalizedSrcRect.setHeight(normalizedSrcRect.height() - (clipRatio * normalizedSrcRect.height()));
> +        normalizedSrcRect.setY(imageRect.y());
> +    }
> +    if ((normalizedSrcRect.y() + normalizedSrcRect.height())  > (imageRect.y() + imageRect.height())) {
> +        float clipRatio = (normalizedSrcRect.height() - (imageRect.y() + imageRect.height() - normalizedSrcRect.y())) / normalizedSrcRect.height();
> +        if (clipRatio >= 1)
> +            return false;
> +        normalizedDstRect.setHeight(normalizedDstRect.height() - (clipRatio * normalizedDstRect.height()));
> +        normalizedSrcRect.setHeight(normalizedSrcRect.height() - (clipRatio * normalizedSrcRect.height()));
> +    }
> +    return true;
> +}

This looks like a lot of source code for such a short sentence :P I don't understand the intention yet. What exactly do you want to do here? Can you upload and attach an example to the bug report? I do not understand why Canvas is working correctly according to comment #1 but other sources are not. Where is the difference? Where do we do something different?

Would be great if you can attach an example and also give feedback how other browsers behave here.

[Rashmi Shyamasundar]

The description at https://bugs.webkit.org/show_bug.cgi?id=67652#c0, given by Mr. Justin Novosad at the time of raising the bug, is not completely correct, with respect to the latest webkit. 

Here is an example to describe this bug :-

Consider the below drawImage call, where "srcImage" can be an image or a canvas or video with width = 60 and height = 60 pixels -
 
canvas.drawImage(srcImage, -30, 0, 60, 30, 0, 0, 60, 30); // left edge clip

In this case - 
source-rectangle : -30, 0, 60, 30
destination-rectangle : 0, 0, 60, 30

For the source-rectangle :-
x = -30
y = 0
width = 60
height = 30

This is same as the sum of 2 rectangles :-
(-30, 0, 30, 30) + (0, 0, 30, 30)

The first rectangle (-30, 0, 30, 30) has no data to fetch from the source Image/Canvas/Video, since the source Image/Canvas/Video has the dimension - 
(0, 0, 60, 60), where the Width or Height is the width or height of the Image/Canvas/Video.

Hence, the first rectangle should be clipped from the source rectangle and the destination-rectangle should also be clipped in the same proportion.

Applying the functions clipRectangles(), defined in my patch :-

clipRatio = 30/60 = 0.5

Final source-rectangle : (0, 0, 30, 30)
Final destnation-rectangle : (30, 30, 30, 30)

Without my patch, the drawImage fails and it does not draw anything.

Please let me know anything is missing in the above description.

[Rashmi Shyamasundar]

Created attachment 206135 [details]
testLeftEdgeClip.html

[Rashmi Shyamasundar]

Created attachment 206136 [details]
testClip.png

[Rashmi Shyamasundar]

Created attachment 206137 [details]
ExpectedResult.png

[Rashmi Shyamasundar]

To test this feature, please download the attached "testLeftEdgeClip.html" and "testClip.png" and open the html in different browsers.

On all the browsers, including the webkit minibrowser, a blank canvas is seen.

If my patch is applied, then the image shown in the attached "ExpectedResult.png" is seen.

[Rashmi Shyamasundar]

The title of this bug is misleading. I think, it should be changed -

FROM :
"Negative width/height values not always treated properly by 2D canvas drawImage"

TO :
"Negative 'x' and 'y' co-ordinate values not treated properly by 2D canvas drawImage"

               OR

Canvas drawImage() does not work as expected when the source-rect specified in the drawImage() call, is not completely inside the source-image.

For example, consider a source-image with width=50, height=50.
Suppose the source-rect is (0, 0, 100, 100) where x=0, y=0, width=100, height=100. Now, source-rect should be right-clipped, and after clipping, the source-rect should be (0, 0, 50, 50).

[Rashmi Shyamasundar]

Created attachment 207691 [details]
ClippingExplained

Please check the attached png image - "ClippingExplained". It shows an example for left-edge clipping. 
Left edge is clipped when the left edge of srcRect(argument for canvas-drawImage function) is not inside the source-image-rect.

[Rashmi Shyamasundar]

Created attachment 207692 [details]
testLeftEdgeClipWithMickey.html

[Rashmi Shyamasundar]

Created attachment 207693 [details]
Mickey_Mouse.png

[Rashmi Shyamasundar]

Please use the attached "testLetEdgeClipWithMickey.html" and "Mickey_Mouse.png" to test the required behavior when left edge of srcRect(argument for canvas-drawImage) is not inside the source-image-rect.

Currently, on all the browsers, drawImage() does not draw anything on the destination-canvas, if the srcRect is not completely inside the source-image-rect.

[Justin Novosad]

(In reply to comment #26)
> Please use the attached "testLetEdgeClipWithMickey.html" and "Mickey_Mouse.png" to test the required behavior when left edge of srcRect(argument for canvas-drawImage) is not inside the source-image-rect.
> 
> Currently, on all the browsers, drawImage() does not draw anything on the destination-canvas, if the srcRect is not completely inside the source-image-rect.

Not true.
Firefox does right thing, and Blink was just recently fixed to comply with the spec. The spec says that the source rectangle must be clipped to the bounds of the source image and the destination rectangle must be clipped proportionately.

Here is the Blink patch that does just that: https://src.chromium.org/viewvc/blink?revision=154798

[Rashmi Shyamasundar]

I checked on Windows 7, in the below browsers -

Mozilla version : 21.0 
Google Chrome version : 28.0
Opera version : 12.12

This spec is not implemented in these browser versions.

[Justin Novosad]

(In reply to comment #28)
> I checked on Windows 7, in the below browsers -
> 
> Mozilla version : 21.0 
> Google Chrome version : 28.0
> Opera version : 12.12
> 
> This spec is not implemented in these browser versions.

It is implemented in Firefox 22 and Chrome 30

[Rashmi Shyamasundar]

Will the patch be automatically picked up from blink - https://chromiumcodereview.appspot.com/19267027 ?

[Justin Novosad]

There is not automatic picking up of Blink patches.  Feel free to apply that patch a against Blink if you want. CanvasRenderderingContext2D probably has not diverged too much between WebKit and Blink, so it should be pretty straightforward.

[Justin Novosad]

(In reply to comment #31)
> There is not automatic picking up of Blink patches.  Feel free to apply that patch a against Blink if you want. CanvasRenderderingContext2D probably has not diverged too much between WebKit and Blink, so it should be pretty straightforward.

I meant "apply that patch a against WebKit", not Blink.  Duh.

[Arunprasad Rajkumar]

It seems to be inactive for almost 2 months. I will take care of merging changes from blink [1][2]

[1] https://codereview.chromium.org/19697021 
[2] https://codereview.chromium.org/19267027

[Rashmi Shyamasundar]

Created attachment 219902 [details]
Patch

[Rashmi Shyamasundar]

As discussed, have taken the above patch from Blink.

[Build Bot]

Comment on attachment 219902 [details]
Patch

Attachment 219902 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/51838180

New failing tests:
fast/canvas/canvas-drawImage-out-of-bounds-src.html

[Build Bot]

Created attachment 219907 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 219902 [details]
Patch

Attachment 219902 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/48298200

New failing tests:
fast/canvas/canvas-drawImage-out-of-bounds-src.html

[Build Bot]

Created attachment 219910 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Daniel Bates]

Comment on attachment 219902 [details]
Patch

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

r- due to the lack of a citation to the Blink commit and the mac-wk2-ews layout test failure.

> Source/WebCore/ChangeLog:10
> +        According to the canvas spec - "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."

Please reference the original Blink commit in the change log entry. We don't seem to have a standard format for such citations at the time of writing. You may either cite the original author of this patch, include the Blink revision number and a URL to the Blink revision as seen in the change log entry for <http://trac.webkit.org/changeset/158676> or you may simply provide a URL to the Blink Git commit as seen in the change log entry for <http://trac.webkit.org/changeset/160747>.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1226
> +static inline void clipRectsToImageRect(const FloatRect& imageRect, FloatRect* srcRect, FloatRect* dstRect)

This function assumes srcRect and dstRect are non-null. Can we make these arguments references instead of pointers? Then we can remove the use of the address-of operator in the caller and the dereference operator in the call to FloatRect::contains() (below).

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1232
> +    FloatSize scale(dstRect->size().width() / srcRect->size().width(), dstRect->size().height() / srcRect->size().height());

Although the compiler is probably smart enough, we may want to consider caching {dstRect, srcRect}->size().{height, width}() in a local variable to avoid extraneous dereferences.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1235
> +    FloatSize offset = dstRect->location() - scaledSrcLocation;

This variable is referenced exactly once (below) and I don't feel its name improves the readability of this code. I suggest we inline the value of this variable on line 1242.

[Daniel Bates]

Comment on attachment 219902 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        must be clipped in the same proportion."
> 
> Please reference the original Blink commit in the change log entry. We don't seem to have a standard format for such citations at the time of writing. You may either cite the original author of this patch, include the Blink revision number and a URL to the Blink revision as seen in the change log entry for <http://trac.webkit.org/changeset/158676> or you may simply provide a URL to the Blink Git commit as seen in the change log entry for <http://trac.webkit.org/changeset/160747>.

On another note, please provide a URL to the section of HTML Canvas 2D Context spec that contains the quoted text, <http://www.w3.org/html/wg/drafts/2dcontext/master/#drawing-images>. You may also want to consider including the draft date (23 December 2013) to make it straightforward to determine the version of the draft that was consulted.

[Daniel Bates]

Comment on attachment 219902 [details]
Patch

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

>>> Source/WebCore/ChangeLog:10
>>> +        must be clipped in the same proportion."
>> 
>> Please reference the original Blink commit in the change log entry. We don't seem to have a standard format for such citations at the time of writing. You may either cite the original author of this patch, include the Blink revision number and a URL to the Blink revision as seen in the change log entry for <http://trac.webkit.org/changeset/158676> or you may simply provide a URL to the Blink Git commit as seen in the change log entry for <http://trac.webkit.org/changeset/160747>.
> 
> On another note, please provide a URL to the section of HTML Canvas 2D Context spec that contains the quoted text, <http://www.w3.org/html/wg/drafts/2dcontext/master/#drawing-images>. You may also want to consider including the draft date (23 December 2013) to make it straightforward to determine the version of the draft that was consulted.

One example of a change log entry that references a spec is: <http://trac.webkit.org/changeset/154005>.

[Rashmi Shyamasundar]

Created attachment 219959 [details]
Patch

[Rashmi Shyamasundar]

Created attachment 219964 [details]
Patch

[Build Bot]

Comment on attachment 219964 [details]
Patch

Attachment 219964 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/47188382

New failing tests:
fast/canvas/canvas-drawImage-out-of-bounds-src.html

[Build Bot]

Created attachment 219965 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Rashmi Shyamasundar]

Created attachment 220105 [details]
Patch

[Rashmi Shyamasundar]

In this patch, I have removed the test for copying a video onto canvas since there is seems to be a problem with loading the video on build bot.

[Brady Eidson]

Comment on attachment 220105 [details]
Patch

Assuming that patches for review since 2013 are stale, r-

[Said Abou-Hallawa]

I think the approach of the attached patch is incorrect. Let's handle this issue in bug 202457

*** This bug has been marked as a duplicate of bug 202457 ***