Bug 65709

On second thought, this bug looks like more of a gray area. The specification explicitly requires the exception to be thrown when source width or source height are zero, but there is no mention of whether or not it should be thrown in other situations. The philosophy of the specification regarding exceptions seems to be to only list exceptions that are required, which makes it kind of vague. The following layouts tests expect an INDEX_SIZE_ERR to be thrown when the sourceRect is out of bounds: canvas/philip/tests/2d.drawImage.outsidesource.html fast/canvas/drawImage-with-invalid-args.html The validity of this interpretation of the spec that led to these tests is debatable. No fix should be submitted against this bug until its validity is correctly ascertained.

Can you make a test page and see what other browsers do? I think in general the spec tries to be explicit about listing every scenario that can throw an exception, since an unexpected exception can break scripts pretty easily.

Normally, I'd point out that the Philip tests in their original form (on the w3c site) have links to an annotated version of the spec, indicating which tests cover which part of the spec. However, http://w3c-test.org/html/tests/submission/PhilipTaylor/canvas/2d.drawImage.outsidesource.html has a link to: http://w3c-test.org/html/tests/submission/PhilipTaylor/annotated-spec/canvas.html#testrefs.2d.drawImage.outsidesource which seems to be a missing anchor. So perhaps the test is now wrong or outdated. Googling for the test name also yields: http://lists.w3.org/Archives/Public/public-html-testsuite/2011Jan/0002.html where Kris Krueger from Microsoft seems to feel the test is also invalid.

@jamesr: So I tried running the layout test in other browsers IE9: Not generating the exceptions for out of bounds src rect Firefox 5.0.1: Inconsistent. Only generates exceptions when out of bound to the left or top (in other words with negative source rect positions) Safari 5.0.4: Generates the exceptions (Duh... WebKit)

+CC Simon Fraser, Oliver Hunt Do you agree we should make this change? Generating more exceptions than the spec requires means that there will be pages that will display fine in IE and fail in WebKit-based browsers.

I think the HTML5 spec needs to document the behavior.

I;m fine removing the exception provided: you verify that resultant behavior is consistent with ie9/firefox, and verify what the spec says should happen. If the spec says an exception should be thrown you should email the working group with a request to remove the exception.

I think Philip's test is either: A) outdated or B) relying on input rules defined in another related spec (like how WebIDL governs a lot of our canvas exceptions). I'm pretty sure it's A) though because I can't spot any part of the specs that document needing this exception here. Personally, I see no compelling reason for an exception here being helpful/needed. Justin, have you started an email thread on the whatwg list or emailed Philip about his test? I would start with doing either/both of those to find out why the test was made that way. Assuming nothing pops up and the test is outdated, we just need to: - remove the test - remove our exceptions - make sure the hosted versions of the test are removed (like the ones of w3c and http://philip.html5.org/tests/canvas/suite/tests/2d.drawImage.outsidesource.html for example) - alert firefox of this

Thanks for the suggestions Matthew. Philip was able to clear-up the issue on whatwg. As you suspected, the test is simply out of date. The spec used to require the exception. That requirement was removed because the inherent instability of floating-point comparisons caused the condition to be flaky. Firefox already adjusted to the change in spec with this: https://bugzilla.mozilla.org/show_bug.cgi?id=664107

Created attachment 103407 [details] Patch

(In reply to comment #10) > Created an attachment (id=103407) [details] > Patch Could someone from Apple comment on whether I am doing the right thing for your test expectations? Basically, I did not change the 'philip' test since it comes from upstream, so I added failed expectations for platform/mac.

Comment on attachment 103407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103407&action=review > LayoutTests/platform/chromium/test_expectations.txt:224 > +BUGWK65709 WONTFIX : canvas/philip/tests/2d.drawImage.outsidesource.html = TEXT Usually we don't add both a bug number and WONTFIX, since that would imply there was a fixable bug. WONTFIX is enough here.

Created attachment 103409 [details] Patch

Comment on attachment 103409 [details] Patch Looks good. Since Oliver r+'ed the previous version, and the only change was for Chromium, I'll r+ this one.

Two things: 1) Justin, you can put the faulty philip test on the mac skipped list at LayoutTests/platform/mac/Skipped so that we ignore it. 2) I just realized something when looking at your patch: It assumes that we aren't supposed to draw any of the image in this case. Are we sure that's correct? I'm not sure the spec is 100% clear, but I'd interpret it otherwise as it currently stands. When I read the current spec, I assumed that not only should we not throw an exception, but we should also actually draw the subregion of the image in this case. Now that I think of it though, it's unclear to me exactly *how* we're supposed to do that. Off the top of my head, I can think of two ways: a) Clip the source rect to the bounds of the image, then draw. b) Clip the source rect to the bounds of the image *after* clipping the dest rect to the subrect that represents the area that the actual image pixels would be drawn if the source rect weren't clipped. So, consider the case with having: - An image with width/height = x - A source rect with top-left point = (-x,-y) and width/height = (3x, 3y) - A destination rect that fills the entire canvas and no more. In case (a), you'd get the image drawn stretched over the entire canvas. In case (b), you'd get the image drawn centered upon the canvas only taking up 1/9 the area of the canvas. Unless I'm missing something in the spec, this sounds like a part of the spec that needs to be more explicit.

That is a good point about the spec being vague. I am pretty sure b) is the correct approach since it respects the image scale and position intended by the original unclipped rectangles. That being said, you just made me realize that I need to change the condition from 'contains' to 'intersects' for the early exit. I should also add a LayoutTest for the use case you described. (In reply to comment #15) > Two things: > > 1) Justin, you can put the faulty philip test on the mac skipped list at LayoutTests/platform/mac/Skipped so that we ignore it. > > 2) I just realized something when looking at your patch: It assumes that we aren't supposed to draw any of the image in this case. Are we sure that's correct? I'm not sure the spec is 100% clear, but I'd interpret it otherwise as it currently stands. > > When I read the current spec, I assumed that not only should we not throw an exception, but we should also actually draw the subregion of the image in this case. Now that I think of it though, it's unclear to me exactly *how* we're supposed to do that. Off the top of my head, I can think of two ways: > > a) Clip the source rect to the bounds of the image, then draw. > b) Clip the source rect to the bounds of the image *after* clipping the dest rect to the subrect that represents the area that the actual image pixels would be drawn if the source rect weren't clipped. > > So, consider the case with having: > - An image with width/height = x > - A source rect with top-left point = (-x,-y) and width/height = (3x, 3y) > - A destination rect that fills the entire canvas and no more. > > In case (a), you'd get the image drawn stretched over the entire canvas. > In case (b), you'd get the image drawn centered upon the canvas only taking up 1/9 the area of the canvas. > > Unless I'm missing something in the spec, this sounds like a part of the spec that needs to be more explicit.

Created attachment 103965 [details] Patch

Attachment 103965 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 ERROR: FAILURES FOR <lucid, x86_64, release, cpu> ERROR: Line:266 Path does not exist. platform/mac-snowleopard/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html LayoutTests/platform/chromium/test_expectations.txt:266: Path does not exist. platform/mac-snowleopard/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html [test/expectations] [2] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #18) The style failure is not from this patch.

Comment on attachment 103965 [details] Patch Running tests with a debug build revealed an assertion. -> review - Investigating... 2011-08-15 17:55:41,470 33060 single_test_runner.py:218 DEBUG worker/1 Stacktrace for fast/canvas/drawImage-with-invalid-args.html: ASSERTION FAILED: CGImageGetHeight(image.get()) == currHeight - CGRectIntegral(srcRect).origin.y /b/build/slave/mac_layout/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../platform/graphics/cg/GraphicsContextCG.cpp(197) : void WebCore::GraphicsContext::drawNativeImage(NativeImagePtr, const WebCore::FloatSize &, WebCore::ColorSpace, const WebCore::FloatRect &, const WebCore::FloatRect &, WebCore::CompositeOperator)

(In reply to comment #20) Looks like a specific code path in GraphicsContextCG was not handling srcRect clipping properly when the image is being scaled and filtered. The layout test was passing on release builds because the source image is just a uniform green rect, so there was no way to detect that it was being blitted at the right scale and position. I will extend the layout test from the last uploaded patch to cover the scaled image cases.

(In reply to comment #15) > a) Clip the source rect to the bounds of the image, then draw. > b) Clip the source rect to the bounds of the image *after* clipping the dest rect to the subrect that represents the area that the actual image pixels would be drawn if the source rect weren't clipped. > > So, consider the case with having: > - An image with width/height = x > - A source rect with top-left point = (-x,-y) and width/height = (3x, 3y) > - A destination rect that fills the entire canvas and no more. > > In case (a), you'd get the image drawn stretched over the entire canvas. > In case (b), you'd get the image drawn centered upon the canvas only taking up 1/9 the area of the canvas. > > Unless I'm missing something in the spec, this sounds like a part of the spec that needs to be more explicit. I'd prefer to do a) in Firefox because I think it's the simpler behaviour to spec and understand. Any interest in doing that in WebKit?

> > I'd prefer to do a) in Firefox because I think it's the simpler behaviour to spec and understand. Any interest in doing that in WebKit? I don't know... I would much rather do the right thing than the easy thing. Ultimately I will leave it up to the spec guys. (Ian?) That being said, I really feel that b) is the right thing to do for several reasons: - It is consistent with how fillStyle works. - It is consistent with how most paint applications (i.e. Photoshop, gimp) handle copying crop regions that fall out of bounds. Therefore, I believe it is the behavior most web devs would expect - Many use cases are pixel-aligned draw operations and it would make sense for them to remain pixel-aligned when the source rect needs to be clipped. - In the case of a source rect with a position animated over time, suddenly stretching the image when we hit a boundary seems like buggy behavior to me. Think of a scrollable image view for example. Besides, implementing this behavior correctly is not that much more work. Here is the algorithm for adjusting the destination rect to match the cropping applied to the source rectangle: xScale = dstRect.width / srcRect.width if srcRect.x < 0 then dstRect.x = dstRect.x - srcRect.x * xScale end if srcRect.maxX > image.width then dstRect.maxX = dstRect.maxX - (srcRect.maxX - image.width) * xScale end ( repeat same algo for Y/height ) srcRect = intersection of srcRect with imageRect

Created attachment 104081 [details] Patch

(In reply to comment #23) > > > > I'd prefer to do a) in Firefox because I think it's the simpler behaviour to spec and understand. Any interest in doing that in WebKit? > > I don't know... I would much rather do the right thing than the easy thing. Ultimately I will leave it up to the spec guys. (Ian?) That being said, I really feel that b) is the right thing to do for several reasons: > - It is consistent with how fillStyle works. > - It is consistent with how most paint applications (i.e. Photoshop, gimp) handle copying crop regions that fall out of bounds. Therefore, I believe it is the behavior most web devs would expect > - Many use cases are pixel-aligned draw operations and it would make sense for them to remain pixel-aligned when the source rect needs to be clipped. > - In the case of a source rect with a position animated over time, suddenly stretching the image when we hit a boundary seems like buggy behavior to me. Think of a scrollable image view for example. > srcRect = intersection of srcRect with imageRect These seem like a pretty good justification.

(In reply to comment #25) > (In reply to comment #23) > > > > > > I'd prefer to do a) in Firefox because I think it's the simpler behaviour to spec and understand. Any interest in doing that in WebKit? > > > > I don't know... I would much rather do the right thing than the easy thing. Ultimately I will leave it up to the spec guys. (Ian?) That being said, I really feel that b) is the right thing to do for several reasons: > > - It is consistent with how fillStyle works. > > - It is consistent with how most paint applications (i.e. Photoshop, gimp) handle copying crop regions that fall out of bounds. Therefore, I believe it is the behavior most web devs would expect > > - Many use cases are pixel-aligned draw operations and it would make sense for them to remain pixel-aligned when the source rect needs to be clipped. > > - In the case of a source rect with a position animated over time, suddenly stretching the image when we hit a boundary seems like buggy behavior to me. Think of a scrollable image view for example. > > srcRect = intersection of srcRect with imageRect > > These seem like a pretty good justification. Agreed. First thing that came to mind was having one canvas used as a "magnifier view" of another canvas - as many games have. I suppose there's also a third option to consider here: C) Don't clip at all, but instead fill in the all the source pixels outside the bounds of the source image with transparent black - a la, getImageData "Pixels outside the canvas must be returned as transparent black." I think that for proper images/videos, B) still makes the most sense to me. However, for drawImage with a canvas element, it *might* be more consistent to use C). Though, I imagine it getting messy with different compositing operations and pixel-twiddling operations - so I'm just putting it out there for completeness.

> C) Don't clip at all, but instead fill in the all the source pixels outside the bounds of the source image with transparent black - a la, getImageData "Pixels outside the canvas must be returned as transparent black." > The only difference in the visual result of C) vs. B) is that B) will produce a hard edge along the clipped boundary, while C) will produce a filtered edge. This difference will only be visible when the draw is not pixel-aligned. When magnifying, C) will cause the image to fade-out along the clipped edges. I am kind of ambivalent between these two options from a functionality point-of-view. However, C) is more costly in terms of performance because it would require the allocation of a temporary image buffer (or texture).

Comment on attachment 104081 [details] Patch Looks good. r=me

Comment on attachment 104081 [details] Patch Clearing flags on attachment: 104081 Committed r93354: <http://trac.webkit.org/changeset/93354>

All reviewed patches have been landed. Closing bug.

[NOOOOO, as I was writing this, the commit bot committed the code! Here's what I was writing:] If we are in fact going with plan B), then its logic (i.e. clipping behavior when src rect is out of bounds) should be in platform independent code (e.g. CanvasRenderingContext2d.cpp) and not down in GraphicsContextCG.cpp. And normally it'd also be a separate patch since it's not necessary to solve the issue of this bug. I'm currently in support of plan B, but if we go that route we also need to update the spec. In particular, this line... "When drawImage() is invoked, the region of the image specified by the source rectangle must be painted on the region of the canvas specified by the destination rectangle, after applying the current transformation matrix to the points of the destination rectangle." ...needs to be updated to something like this (but of course written cleaner) ... "When drawImage() is invoked, the region of the image specified by the source rectangle must be painted on the region of the canvas specified by the destination rectangle, after applying the current transformation matrix to the points of the destination rectangle. Pixels values for the source rectangle that lie outside the bounds of the source image should be transparent black." (Technically, that specifies plan C, but I just wrote it for simplicity here.) Ideally, this change should not go in until the spec is updated to document this behavior. There's a chance that we're missing some subtle issue here that could be uncovered through getting the input of others in changing the spec. Though, if this change doesn't affect any tests other than the faulty/old DOM Exception test, I don't see much of an issue.

Reopening to address the issues I raised.

Sorry, Matthew, My bad. Justin felt we had consensus that choice (B) was acceptable. I'll revert the change and we can continue the discussion. (In reply to comment #31) > Ideally, this change should not go in until the spec is updated to document this behavior. There's a chance that we're missing some subtle issue here that could be uncovered through getting the input of others in changing the spec. > > Though, if this change doesn't affect any tests other than the faulty/old DOM Exception test, I don't see much of an issue. Justin tells me it doesn't affect other tests, but if not, the bots should tell us soon enough.

This patch appears to have caused a test failure on Qt: http://build.webkit.org/builders/Qt%20Linux%20Release/builds/36614 http://build.webkit.org/results/Qt%20Linux%20Release/r93357%20(36614)/results.html

> If we are in fact going with plan B), then its logic (i.e. clipping behavior when src rect is out of bounds) should be in platform independent code (e.g. CanvasRenderingContext2d.cpp) and not down in GraphicsContextCG.cpp. I agree, the only reason I did it the other way was that the platforms I was testing (CG and Skia) already had the logic for plan B) in them, believe it or not. Only exception was the magnified filtering case that needed fixing up in GraphicsContextCG.cpp >And normally it'd also be a separate patch since it's not necessary to solve the issue of this bug. > Fair enough > Ideally, this change should not go in until the spec is updated to document this behavior. There's a chance that we're missing some subtle issue here that could be uncovered through getting the input of others in changing the spec. > The problem is that we can't remove the exception without resolving the question of what we want to do instead of the exception (i.e. what do we render ?). > Though, if this change doesn't affect any tests other than the faulty/old DOM Exception test, I don't see much of an issue. Actually, that test (fast/canvas/drawImage-with-invalid-args.html) asserts on mac if we don't do anything about clipping. As an interim solution, until plan B) is universally agreed upon and implemented, we could just render nothing. In other words, exit if the image rect does not contain the source rectangle entirely, which is no worse that the current situation. In a later patch, once the spec is cleared up, we'd take care of the intersection case. Does that sound reasonable?

The issue of what to do about partially out of bounds source rectangles was spun-off here: https://bugs.webkit.org/show_bug.cgi?id=66574

Comment on attachment 104081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104081&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1371 > + FloatRect normalizedSrcRect = normalizeRect(srcRect); Why is normalizing the rect here needed for this fix? Do the intersection tests fail if you don't normalize? (Same comment for all other normalizations below) > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:-199 > - } What's the justification for removing this? I believe this is for partially loaded images, no? Looking quickly, I don't see how your above code compensates for that.

> Why is normalizing the rect here needed for this fix? Do the intersection tests fail if you don't normalize? (Same comment for all other normalizations below) Ah yes, this is a problem that surface when I wrote the layout test. The problem is that negative width an height values were not handled correctly for image to canvas and video to canvas draws. > > > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:-199 > > - } > > What's the justification for removing this? I believe this is for partially loaded images, no? Looking quickly, I don't see how your above code compensates for that. This check is no longer necessary (will no longer be hit) because of the additional clipping performed above. The ASSERT I added above double-checks this.

(In reply to comment #38) >The problem is that negative width an height values were not handled correctly for image to canvas and video to canvas draws. > This was spun-off into a separate bug: https://bugs.webkit.org/show_bug.cgi?id=67652 For this bug, I will submit a new patch that addresses only the DOM exception.

Created attachment 106485 [details] Patch

Looks good, but I'd like approval from Matthew or one of the other Apple folks on this bug to take a look before r+.

Comment on attachment 106485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106485&action=review Looks good to me. Has the philip test been updated for his hosted version or the test.w3.org submitted one? > Source/WebCore/ChangeLog:3 > + Unwarranted DOM Exception when when canvas2D drawImage is called with src when when > Source/WebCore/ChangeLog:11 > + Silently fail when source rectangle is out of bounds Return early without throwing an exception if source rectangle is out of bounds to match the spec.

Related mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=655328

Created attachment 107065 [details] Patch

Pinging reviewers. Patch is going stale here.

Comment on attachment 107065 [details] Patch Clearing flags on attachment: 107065 Committed r95899: <http://trac.webkit.org/changeset/95899>

All reviewed patches have been landed. Closing bug.