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
: WebKit
Canvas
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 65709 88236
:
  Show dependency treegraph
 
Reported: 2011-08-19 12:27 PST by
Modified: 2014-04-04 06:12 PST (History)


Attachments
Patch (35.72 KB, patch)
2012-01-31 08:13 PST, Justin Novosad
no flags Review Patch | Details | Formatted Diff | Diff
Patch (35.72 KB, patch)
2012-01-31 11:05 PST, Justin Novosad
no flags Review Patch | Details | Formatted Diff | Diff
Patch (35.68 KB, patch)
2012-01-31 11:21 PST, Justin Novosad
no flags Review Patch | Details | Formatted Diff | Diff
Patch (36.21 KB, patch)
2012-02-01 12:29 PST, Justin Novosad
no flags Review Patch | Details | Formatted Diff | Diff
Patch (102.72 KB, patch)
2012-05-29 08:00 PST, Justin Novosad
no flags Review Patch | Details | Formatted Diff | Diff
Patch (102.71 KB, patch)
2012-05-29 10:21 PST, Justin Novosad
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (658.79 KB, application/zip)
2012-05-29 14:39 PST, WebKit Review Bot
no flags Details
Patch (102.71 KB, patch)
2012-05-30 06:50 PST, Justin Novosad
no flags Review Patch | Details | Formatted Diff | Diff
Patch (35.45 KB, patch)
2012-06-04 11:22 PST, Justin Novosad
senorblanco: review+
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cq-02 (2.35 MB, application/zip)
2012-06-04 16:08 PST, WebKit Review Bot
no flags Details


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-08-19 12:27:54 PST
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 From 2011-08-24 15:09:18 PST -------
Why aren't pixels that fall outside the src image boundary just considered transparent?
------- Comment #2 From 2011-09-06 10:07:31 PST -------
(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 From 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 From 2012-01-31 08:13:13 PST -------
Created an attachment (id=124737) [details]
Patch
------- Comment #5 From 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 From 2012-01-31 09:16:24 PST -------
(From update of attachment 124737 [details])
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 From 2012-01-31 11:05:00 PST -------
Created an attachment (id=124777) [details]
Patch
------- Comment #8 From 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] [details]
> Patch
------- Comment #9 From 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 From 2012-01-31 11:12:08 PST -------
(From update of attachment 124777 [details])
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 From 2012-01-31 11:21:45 PST -------
Created an attachment (id=124781) [details]
Patch
------- Comment #12 From 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 From 2012-01-31 12:19:32 PST -------
(From update of attachment 124781 [details])
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 From 2012-02-01 12:29:51 PST -------
Created an attachment (id=124989) [details]
Patch
------- Comment #15 From 2012-02-07 07:29:55 PST -------
Pinging reviewers.
------- Comment #16 From 2012-05-29 08:00:52 PST -------
Created an attachment (id=144560) [details]
Patch
------- Comment #17 From 2012-05-29 08:03:53 PST -------
(In reply to comment #16)
> Created an attachment (id=144560) [details] [details]
> Patch

Previous patch got stale.  Refreshed it.

Reviewers: Anybody home?
------- Comment #18 From 2012-05-29 09:45:17 PST -------
(From update of attachment 144560 [details])
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 From 2012-05-29 10:21:46 PST -------
Created an attachment (id=144581) [details]
Patch
------- Comment #20 From 2012-05-29 13:29:19 PST -------
(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?
------- Comment #21 From 2012-05-29 13:56:00 PST -------
(In reply to comment #20)
> (From update of attachment 144581 [details] [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 From 2012-05-29 14:39:11 PST -------
(From update of attachment 144581 [details])
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 From 2012-05-29 14:39:16 PST -------
Created an attachment (id=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 From 2012-05-30 06:50:38 PST -------
Created an attachment (id=144797) [details]
Patch
------- Comment #25 From 2012-05-30 06:56:42 PST -------
(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 From 2012-06-04 07:35:21 PST -------
(From update of attachment 144797 [details])
OK.  r=me
------- Comment #27 From 2012-06-04 07:44:09 PST -------
(From update of attachment 144797 [details])
Clearing flags on attachment: 144797

Committed r119393: <http://trac.webkit.org/changeset/119393>
------- Comment #28 From 2012-06-04 07:44:17 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #29 From 2012-06-04 08:55:24 PST -------
Re-opened since this is blocked by 88236
------- Comment #30 From 2012-06-04 08:58:00 PST -------
The test as committed has duplicate content. Apparently SVN can do this sometimes if you add a file twice.
------- Comment #31 From 2012-06-04 11:22:17 PST -------
Created an attachment (id=145609) [details]
Patch
------- Comment #32 From 2012-06-04 11:25:37 PST -------
(From update of attachment 145609 [details])
One mo' time!  r=me
------- Comment #33 From 2012-06-04 16:08:39 PST -------
(From update of attachment 145609 [details])
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 From 2012-06-04 16:08:44 PST -------
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
------- Comment #35 From 2012-06-05 10:54:33 PST -------
(In reply to comment #34)
> Created an attachment (id=145646) [details] [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 From 2012-06-05 11:00:55 PST -------
(From update of attachment 145609 [details])
Trying CQ+ again, to see if the css failure was flake.
------- Comment #37 From 2012-06-05 11:13:54 PST -------
(From update of attachment 145609 [details])
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 From 2014-04-03 02:36:05 PST -------
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 From 2014-04-04 06:12:29 PST -------
> 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.