Bug 66574 - Canvas 2D - Source rectangles that overlap the source image boundary, not supported by drawImage
: Canvas 2D - Source rectangles that overlap the source image boundary, not sup...
Status: REOPENED
Product: WebKit
Classification: Unclassified
Component: Canvas
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nobody
:
Depends on: 65709 88236
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-19 12:27 PDT by Justin Novosad
Modified: 2014-04-04 06:12 PDT (History)
12 users (show)

See Also:


Attachments
Patch (35.72 KB, patch)
2012-01-31 08:13 PST, Justin Novosad
no flags Details | Formatted Diff | Diff
Patch (35.72 KB, patch)
2012-01-31 11:05 PST, Justin Novosad
no flags Details | Formatted Diff | Diff
Patch (35.68 KB, patch)
2012-01-31 11:21 PST, Justin Novosad
no flags Details | Formatted Diff | Diff
Patch (36.21 KB, patch)
2012-02-01 12:29 PST, Justin Novosad
no flags Details | Formatted Diff | Diff
Patch (102.72 KB, patch)
2012-05-29 08:00 PDT, Justin Novosad
no flags Details | Formatted Diff | Diff
Patch (102.71 KB, patch)
2012-05-29 10:21 PDT, Justin Novosad
no flags Details | Formatted Diff | Diff
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 Details
Patch (102.71 KB, patch)
2012-05-30 06:50 PDT, Justin Novosad
no flags Details | Formatted Diff | Diff
Patch (35.45 KB, patch)
2012-06-04 11:22 PDT, Justin Novosad
senorblanco: review+
webkit.review.bot: commit‑queue-
Details | Formatted Diff | Diff
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 Details

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Novosad 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).
Comment 1 Simon Fraser (smfr) 2011-08-24 15:09:18 PDT
Why aren't pixels that fall outside the src image boundary just considered transparent?
Comment 2 Justin Novosad 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.
Comment 3 Justin Novosad 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
Comment 4 Justin Novosad 2012-01-31 08:13:13 PST
Created attachment 124737 [details]
Patch
Comment 5 WebKit Review Bot 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.
Comment 6 WebKit Review Bot 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
Comment 7 Justin Novosad 2012-01-31 11:05:00 PST
Created attachment 124777 [details]
Patch
Comment 8 Justin Novosad 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
Comment 9 WebKit Review Bot 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.
Comment 10 Stephen White 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.
Comment 11 Justin Novosad 2012-01-31 11:21:45 PST
Created attachment 124781 [details]
Patch
Comment 12 WebKit Review Bot 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.
Comment 13 WebKit Review Bot 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
Comment 14 Justin Novosad 2012-02-01 12:29:51 PST
Created attachment 124989 [details]
Patch
Comment 15 Justin Novosad 2012-02-07 07:29:55 PST
Pinging reviewers.
Comment 16 Justin Novosad 2012-05-29 08:00:52 PDT
Created attachment 144560 [details]
Patch
Comment 17 Justin Novosad 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?
Comment 18 Darin Adler 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.
Comment 19 Justin Novosad 2012-05-29 10:21:46 PDT
Created attachment 144581 [details]
Patch
Comment 20 Darin Adler 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?
Comment 21 Justin Novosad 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
Comment 22 WebKit Review Bot 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
Comment 23 WebKit Review Bot 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
Comment 24 Justin Novosad 2012-05-30 06:50:38 PDT
Created attachment 144797 [details]
Patch
Comment 25 Justin Novosad 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 :-))
Comment 26 Stephen White 2012-06-04 07:35:21 PDT
Comment on attachment 144797 [details]
Patch

OK.  r=me
Comment 27 WebKit Review Bot 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>
Comment 28 WebKit Review Bot 2012-06-04 07:44:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 WebKit Review Bot 2012-06-04 08:55:24 PDT
Re-opened since this is blocked by 88236
Comment 30 Stephen Chenney 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.
Comment 31 Justin Novosad 2012-06-04 11:22:17 PDT
Created attachment 145609 [details]
Patch
Comment 32 Stephen White 2012-06-04 11:25:37 PDT
Comment on attachment 145609 [details]
Patch

One mo' time!  r=me
Comment 33 WebKit Review Bot 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
Comment 34 WebKit Review Bot 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
Comment 35 Justin Novosad 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.
Comment 36 Stephen White 2012-06-05 11:00:55 PDT
Comment on attachment 145609 [details]
Patch

Trying CQ+ again, to see if the css failure was flake.
Comment 37 WebKit Review Bot 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
Comment 38 Dirk Schulze 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?
Comment 39 Justin Novosad 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.