Bug 67652

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

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

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

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.

It's all yours.

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.

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

Created attachment 205262 [details] Patch

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.

Created attachment 205394 [details] Patch

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

Created attachment 205451 [details] Patch

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 ?

Created attachment 205773 [details] Patch

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

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.

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.

Created attachment 206135 [details] testLeftEdgeClip.html

Created attachment 206136 [details] testClip.png

Created attachment 206137 [details] ExpectedResult.png

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.

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

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.

Created attachment 207692 [details] testLeftEdgeClipWithMickey.html

Created attachment 207693 [details] Mickey_Mouse.png

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.

(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

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.

(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

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

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.

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

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

Created attachment 219902 [details] Patch

As discussed, have taken the above patch from Blink.

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

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

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

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

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.

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.

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

Created attachment 219959 [details] Patch

Created attachment 219964 [details] Patch

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

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

Created attachment 220105 [details] Patch

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.

Comment on attachment 220105 [details] Patch Assuming that patches for review since 2013 are stale, r-

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 ***