REOPENED 66574
Canvas 2D - Source rectangles that overlap the source image boundary, not supported by drawImage
https://bugs.webkit.org/show_bug.cgi?id=66574
Summary Canvas 2D - Source rectangles that overlap the source image boundary, not sup...
Justin Novosad
Reported 2011-08-19 12:27:54 PDT
This bug is a spin-off of https://bugs.webkit.org/show_bug.cgi?id=65709 Removing the DOM exception that was thrown by drawImage when the source rectangle is out of bounds exposed the question of how to handle source rectangles that are only partially out of bounds In the discussion for bug 65709, we agreed that the out-of bound region should be treated as transparent black. There was still an open question about whether this result should be achieved by a) clipping the the source rectangle to the bounds of the source image, and clipping the destination rectangle in such a way that preserves the src->dst coordinate mapping; or b) padding the source rectangle with transparent black pixels. This would cause the encroached source image edges to be subject to filtering (i.e. not treated as border pixels) Currently, the HTML spec does not provide a clear answer as to what the correct approach should be. The resolution of this bug is currently blocked on resolving the ambiguity in the HTML5 canvas element specification The question was raised on whatwg : http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-August/032849.html But this thread did not yield a satisfying answer (yet).
Attachments
Patch (35.72 KB, patch)
2012-01-31 08:13 PST, Justin Novosad
no flags
Patch (35.72 KB, patch)
2012-01-31 11:05 PST, Justin Novosad
no flags
Patch (35.68 KB, patch)
2012-01-31 11:21 PST, Justin Novosad
no flags
Patch (36.21 KB, patch)
2012-02-01 12:29 PST, Justin Novosad
no flags
Patch (102.72 KB, patch)
2012-05-29 08:00 PDT, Justin Novosad
no flags
Patch (102.71 KB, patch)
2012-05-29 10:21 PDT, Justin Novosad
no flags
Archive of layout-test-results from ec2-cr-linux-04 (658.79 KB, application/zip)
2012-05-29 14:39 PDT, WebKit Review Bot
no flags
Patch (102.71 KB, patch)
2012-05-30 06:50 PDT, Justin Novosad
no flags
Patch (35.45 KB, patch)
2012-06-04 11:22 PDT, Justin Novosad
senorblanco: review+
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cq-02 (2.35 MB, application/zip)
2012-06-04 16:08 PDT, WebKit Review Bot
no flags
Simon Fraser (smfr)
Comment 1 2011-08-24 15:09:18 PDT
Why aren't pixels that fall outside the src image boundary just considered transparent?
Justin Novosad
Comment 2 2011-09-06 10:07:31 PDT
(In reply to comment #1) > Why aren't pixels that fall outside the src image boundary just considered transparent? That's the idea. Proposals a) and b) are just different implementation strategies for of achieving that, which yield subtly different results with respect to edge filtering.
Justin Novosad
Comment 3 2012-01-24 13:14:55 PST
The proper behavior for this case is now clarified in the spec: http://html5.org/tools/web-apps-tracker?from=6907&to=6908
Justin Novosad
Comment 4 2012-01-31 08:13:13 PST
WebKit Review Bot
Comment 5 2012-01-31 08:15:25 PST
Attachment 124737 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 6 2012-01-31 09:16:24 PST
Comment on attachment 124737 [details] Patch Attachment 124737 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11372376 New failing tests: fast/canvas/drawImage-clipped-source.html
Justin Novosad
Comment 7 2012-01-31 11:05:00 PST
Justin Novosad
Comment 8 2012-01-31 11:06:14 PST
Same as previous patch: retrying EWS because failures seem unrelated or flake. (In reply to comment #7) > Created an attachment (id=124777) [details] > Patch
WebKit Review Bot
Comment 9 2012-01-31 11:07:41 PST
Attachment 124777 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit 9fc4aeb..516488a master -> origin/master Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 106368 = 9fc4aeb5a25445667675b8be04139afe13128677 r106369 = 516488acc916a40a28a51c73fcf5ad5640b39f3d Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
Stephen White
Comment 10 2012-01-31 11:12:08 PST
Comment on attachment 124777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124777&action=review > Source/WebCore/ChangeLog:13 > + is clipped, and the destination rectangle is clipped propartionally. Nit: propartionally -> proportionally? (or proportionately) > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1264 > +static inline bool normalizeAndTestRects(FloatRect imageRect, FloatRect& srcRect, FloatRect& dstRect, ExceptionCode& ec) Could pass the imageRect by const ref, instead of by value. TestRects against what? maybe normalizeAndClipRects would be a better name.
Justin Novosad
Comment 11 2012-01-31 11:21:45 PST
WebKit Review Bot
Comment 12 2012-01-31 11:23:21 PST
Attachment 124781 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 13 2012-01-31 12:19:32 PST
Comment on attachment 124781 [details] Patch Attachment 124781 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11387084 New failing tests: fast/canvas/drawImage-clipped-source.html
Justin Novosad
Comment 14 2012-02-01 12:29:51 PST
Justin Novosad
Comment 15 2012-02-07 07:29:55 PST
Pinging reviewers.
Justin Novosad
Comment 16 2012-05-29 08:00:52 PDT
Justin Novosad
Comment 17 2012-05-29 08:03:53 PDT
(In reply to comment #16) > Created an attachment (id=144560) [details] > Patch Previous patch got stale. Refreshed it. Reviewers: Anybody home?
Darin Adler
Comment 18 2012-05-29 09:45:17 PDT
Comment on attachment 144560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144560&action=review Looks OK. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1260 > +static inline bool normalizeAndClipRects(const FloatRect &imageRect, FloatRect& srcRect, FloatRect& dstRect, ExceptionCode& ec) The & for imageRect is in the wrong place for WebKit coding style. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1264 > + if (!isfinite(dstRect.x()) || !isfinite(dstRect.y()) || !isfinite(dstRect.width()) || !isfinite(dstRect.height()) > + || !isfinite(srcRect.x()) || !isfinite(srcRect.y()) || !isfinite(srcRect.width()) || !isfinite(srcRect.height())) > + return false; I’d like to see a helper function that does this kind of check on a FloatRect so we can write this if statement in a more compact way. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1267 > + FloatRect origSrcRect = srcRect; I suggest the whole word “original” rather than the abbreviation “orig”. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1269 > + if (!srcRect.width() || !srcRect.height()) { I think it would be better to write this as: if (srcRect.isEmpty()) Or srcRect.isZero(). > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1275 > + if (!dstRect.width() || !dstRect.height()) > + return false; I think it would be better to write this as: if (dstRect.isEmpty()) Or dstRect.isZero(). > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1347 > + if (!normalizeAndClipRects(imageRect, normalizedSrcRect, normalizedDstRect, ec)) { > return; > } Single line if statement bodies don’t get braces in WebKit coding style.
Justin Novosad
Comment 19 2012-05-29 10:21:46 PDT
Darin Adler
Comment 20 2012-05-29 13:29:19 PDT
Comment on attachment 144581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144581&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1273 > + if (srcRect.isEmpty()) { Do we have test coverage for negative sizes? > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1278 > + if (dstRect.isEmpty()) Ditto?
Justin Novosad
Comment 21 2012-05-29 13:56:00 PDT
(In reply to comment #20) > (From update of attachment 144581 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144581&action=review > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1273 > > + if (srcRect.isEmpty()) { > > Do we have test coverage for negative sizes? > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1278 > > + if (dstRect.isEmpty()) > > Ditto? srcRect with negative size is covered by the test included in this patch. See: fast/canvas/drawImage-clipped-source.js, line 70. The case of dstRect with negative size is not covered here because it does not impact source rect clipping. Other existing layout tests are covering that case, e.g. fast/canvas/drawImage-with-negative-source-destination.html
WebKit Review Bot
Comment 22 2012-05-29 14:39:11 PDT
Comment on attachment 144581 [details] Patch Attachment 144581 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12850575 New failing tests: platform/chromium/virtual/gpu/canvas/philip/tests/2d.drawImage.negativedest.html platform/chromium/virtual/gpu/canvas/philip/tests/2d.drawImage.negativedir.html canvas/philip/tests/2d.drawImage.negativedir.html canvas/philip/tests/2d.drawImage.negativedest.html
WebKit Review Bot
Comment 23 2012-05-29 14:39:16 PDT
Created attachment 144623 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Justin Novosad
Comment 24 2012-05-30 06:50:38 PDT
Justin Novosad
Comment 25 2012-05-30 06:56:42 PDT
(In reply to comment #18) > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1275 > > + if (!dstRect.width() || !dstRect.height()) > > + return false; > > I think it would be better to write this as: > > if (dstRect.isEmpty()) > > Or dstRect.isZero(). > Previous patch failed a test because I used isEmpty (oops). isZero was the right choice ( width/height ==0 vs. <=0). Corrected in latest patch. That test failure also shows that we do in fact have test coverage for cases where dstRect has negative dimensions :-))
Stephen White
Comment 26 2012-06-04 07:35:21 PDT
Comment on attachment 144797 [details] Patch OK. r=me
WebKit Review Bot
Comment 27 2012-06-04 07:44:09 PDT
Comment on attachment 144797 [details] Patch Clearing flags on attachment: 144797 Committed r119393: <http://trac.webkit.org/changeset/119393>
WebKit Review Bot
Comment 28 2012-06-04 07:44:17 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 29 2012-06-04 08:55:24 PDT
Re-opened since this is blocked by 88236
Stephen Chenney
Comment 30 2012-06-04 08:58:00 PDT
The test as committed has duplicate content. Apparently SVN can do this sometimes if you add a file twice.
Justin Novosad
Comment 31 2012-06-04 11:22:17 PDT
Stephen White
Comment 32 2012-06-04 11:25:37 PDT
Comment on attachment 145609 [details] Patch One mo' time! r=me
WebKit Review Bot
Comment 33 2012-06-04 16:08:39 PDT
Comment on attachment 145609 [details] Patch Rejecting attachment 145609 [details] from commit-queue. New failing tests: css1/box_properties/border_left_width.html Full output: http://queues.webkit.org/results/12892444
WebKit Review Bot
Comment 34 2012-06-04 16:08:44 PDT
Created attachment 145646 [details] Archive of layout-test-results from ec2-cq-02 The attached test failures were seen while running run-webkit-tests on the commit-queue. Bot: ec2-cq-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Justin Novosad
Comment 35 2012-06-05 10:54:33 PDT
(In reply to comment #34) > Created an attachment (id=145646) [details] > Archive of layout-test-results from ec2-cq-02 > > The attached test failures were seen while running run-webkit-tests on the commit-queue. > Bot: ec2-cq-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick Failure looks unrelated. Probable flake.
Stephen White
Comment 36 2012-06-05 11:00:55 PDT
Comment on attachment 145609 [details] Patch Trying CQ+ again, to see if the css failure was flake.
WebKit Review Bot
Comment 37 2012-06-05 11:13:54 PDT
Comment on attachment 145609 [details] Patch Rejecting attachment 145609 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: queue/Source/WebKit/chromium/third_party/skia/include --revision 4138 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 43>At revision 4138. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/12896622
Dirk Schulze
Comment 38 2014-04-03 02:36:05 PDT
Was this bug landed? Just to confirm if I understand it correctly. The bug was about an source rect that might be bigger than the source image, correct?
Justin Novosad
Comment 39 2014-04-04 06:12:29 PDT
> Just to confirm if I understand it correctly. The bug was about an source rect > that might be bigger than the source image, correct? Yes, but to be more accurate the rect is not necessarily bigger, it is when the source rect covers pixels that are outside of the source image.
Note You need to log in before you can comment on or make changes to this bug.