Canvas 2D - Source rectangles that overlap the source image boundary, not supported by drawImage

• Component: Canvas
• Status: REOPENED ---

[Justin Novosad]

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

[Simon Fraser (smfr)]

Why aren't pixels that fall outside the src image boundary just considered transparent?

[Justin Novosad]

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

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]

Created attachment 124737 [details]
Patch

[WebKit Review Bot]

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

Created attachment 124777 [details]
Patch

[Justin Novosad]

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]

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

Created attachment 124781 [details]
Patch

[WebKit Review Bot]

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

Created attachment 124989 [details]
Patch

[Justin Novosad]

Pinging reviewers.

[Justin Novosad]

Created attachment 144560 [details]
Patch

[Justin Novosad]

(In reply to comment #16)
> Created an attachment (id=144560) [details]
> Patch

Previous patch got stale.  Refreshed it.

Reviewers: Anybody home?

[Darin Adler]

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]

Created attachment 144581 [details]
Patch

[Darin Adler]

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]

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

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]

Created attachment 144797 [details]
Patch

[Justin Novosad]

(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 on attachment 144797 [details]
Patch

OK.  r=me

[WebKit Review Bot]

Comment on attachment 144797 [details]
Patch

Clearing flags on attachment: 144797

Committed r119393: <http://trac.webkit.org/changeset/119393>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 88236

[Stephen Chenney]

The test as committed has duplicate content. Apparently SVN can do this sometimes if you add a file twice.

[Justin Novosad]

Created attachment 145609 [details]
Patch

[Stephen White]

Comment on attachment 145609 [details]
Patch

One mo' time!  r=me

[WebKit Review Bot]

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]

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]

(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 on attachment 145609 [details]
Patch

Trying CQ+ again, to see if the css failure was flake.

[WebKit Review Bot]

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]

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]

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