66920 – Canvas drawImage with SourceIn, DestinationIn, SourceOut, DestinationAtop and Copy have errors

RESOLVED FIXED 66920

Canvas drawImage with SourceIn, DestinationIn, SourceOut, DestinationAtop and Copy have errors

https://bugs.webkit.org/show_bug.cgi?id=66920

Summary Canvas drawImage with SourceIn, DestinationIn, SourceOut, DestinationAtop and...

Ben Wells

Reported 2011-08-24 20:42:52 PDT

Created attachment 105127 [details] Test case These have the same errors as were recently fixed in bug 66036.

Attachments
Test case (5.18 KB, text/html) 2011-08-24 20:42 PDT, Ben Wells	no flags	Details
Patch for comment, patch with tests to come. (10.67 KB, patch) 2011-10-13 23:57 PDT, Ben Wells	no flags	Details Formatted Diff Diff
Patch which doesn't merge. (39.52 KB, patch) 2011-10-16 19:22 PDT, Ben Wells	no flags	Details Formatted Diff Diff
Patch with function made static non-member, and test added. (39.60 KB, patch) 2011-10-16 20:26 PDT, Ben Wells	no flags	Details Formatted Diff Diff
Patch (38.32 KB, patch) 2011-10-18 17:00 PDT, Ben Wells	no flags	Details Formatted Diff Diff
Patch (49.57 KB, patch) 2011-10-23 21:11 PDT, Ben Wells	no flags	Details Formatted Diff Diff
Test updated to pass on all platforms. (43.87 KB, patch) 2011-10-25 22:40 PDT, Ben Wells	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Tom Hudson

Comment 1 2011-09-01 13:26:56 PDT

Working on this.

Ben Wells

Comment 2 2011-10-13 23:57:08 PDT

Created attachment 110970 [details] Patch for comment, patch with tests to come.

Ben Wells

Comment 3 2011-10-14 00:04:07 PDT

While doing this patch I realised there might be some problems with fill in these modes: * didDraw is called with a rect which is too small (it should be called on the entire canvas), * if the buffer is clipped to the canvas bounds there so that the location changes, the fill will happen in the wrong location. I'm planning to fix these in a couple of subsequent patches. Also, this patch only fixes drawImage with an image, not a canvas or video element. I'm also planning to handle those cases in a subsequent patch.

Tom Hudson

Comment 4 2011-10-14 11:16:01 PDT

Comment on attachment 110970 [details] Patch for comment, patch with tests to come. View in context: https://bugs.webkit.org/attachment.cgi?id=110970&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1521 > +bool CanvasRenderingContext2D::isFullCanvasCompositeMode(CompositeOperator op) const This is a better interface. The name's not clear at first glance, but when you see the function on the next line it makes enough sense, and doesCompositeModeRequireFullCanvas() is a bit wordy. Also, does it even need to be a member? Does anybody outside the class want to access it? Or can it be a static helper?

Darin Adler

Comment 5 2011-10-14 11:41:52 PDT

Comment on attachment 110970 [details] Patch for comment, patch with tests to come. View in context: https://bugs.webkit.org/attachment.cgi?id=110970&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1521 >> +bool CanvasRenderingContext2D::isFullCanvasCompositeMode(CompositeOperator op) const > > This is a better interface. The name's not clear at first glance, but when you see the function on the next line it makes enough sense, and doesCompositeModeRequireFullCanvas() is a bit wordy. Also, does it even need to be a member? Does anybody outside the class want to access it? Or can it be a static helper? This function no longer makes sense as a non-static member function. It can be a standalone function, internal to the file (what Tom calls a static helper), or a static member function.

Ben Wells

Comment 6 2011-10-16 19:22:52 PDT

Created attachment 111198 [details] Patch which doesn't merge.

Ben Wells

Comment 7 2011-10-16 20:26:07 PDT

Created attachment 111202 [details] Patch with function made static non-member, and test added.

James Robinson

Comment 8 2011-10-18 15:56:04 PDT

Comment on attachment 111202 [details] Patch with function made static non-member, and test added. View in context: https://bugs.webkit.org/attachment.cgi?id=111202&action=review Some initial comments. Please file a bug about didDraw() bounds for the other draw paths, it seems that they are wrong (and this is correct). I nominate someone else to check the math here - everything else looks good. Stephen perhaps? > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1335 > + didDraw(FloatRect(0, 0, canvas()->width(), canvas()->height()), CanvasDidDrawApplyClip); nit: FloatRect(FloatPoint::zero(), canvas()->size()) or FloatRect(FloatPoint(), canvas()->size()) would be slightly terser and less prone to error > LayoutTests/fast/canvas/canvas-composite-image.html:1 > +<html> add <!DOCTYPE html> above this, please > LayoutTests/fast/canvas/canvas-composite-image.html:5 > + <meta name="DC.creator" content="Kamiel Martinet, http://www.martinet.nl/"> > + <meta name="DC.publisher" content="Mozilla Developer Center, http://developer.mozilla.org"> these lines are kind of odd. Was this page actually generated?

Ben Wells

Comment 9 2011-10-18 17:00:30 PDT

Created attachment 111529 [details] Patch

Ben Wells

Comment 10 2011-10-18 17:03:52 PDT

Webkit bug 70379 logged for didDraw calls in fills. Also test updated to remove cruft from the original test I modified to create this one, and add changes for other feedback.

Stephen White

Comment 11 2011-10-19 13:42:00 PDT

Comment on attachment 111529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111529&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1338 > + } else if (op == CompositeCopy) { > + clearCanvas(); > + c->drawImage(cachedImage->imageForRenderer(image->renderer()), ColorSpaceDeviceRGB, normalizedDstRect, normalizedSrcRect, op); It seems like we could do better than this in the Copy case. If the destination rect covers the full canvas already, this looks like it will do an unnecessary clear. (It looks like fill() and fillRect() also have this problem). Beyond that, a further optimization would be to clear only the region outside the destination rect. You can either of these in a future patch, but I would like to see at least the first optimization done at some point. > LayoutTests/fast/canvas/canvas-composite-image.html:12 > + // This test should show a table of canvas elements. Onto the background of each a blue > + // square is drawn, either fully opaque or with some transparency, and then a clip and > + // transformation is applied. The clip only allows drawing in the left two thirds of the > + // canvas. I would really prefer that we not introduce new canvas tests with pixel results, if possible. Pixels should be tested programmatically in JavaScript, using the fast/canvas/script-tests infrastructure. This avoids having to maintain platform-specific image baselines. In fact, doesn't the canvas/philip suite cover these modes? Or do they not have enough coverage? > LayoutTests/platform/chromium/test_expectations.txt:3588 > +BUGWK66920 MAC WIN : fast/canvas/canvas-composite-image.html = IMAGE Doesn't this also affect Linux?

Ben Wells

Comment 12 2011-10-23 21:11:18 PDT

Created attachment 112138 [details] Patch

Ben Wells

Comment 13 2011-10-23 21:20:10 PDT

(In reply to comment #11) > It seems like we could do better than this in the Copy case. If the destination rect covers the full canvas already, this looks like it will do an unnecessary clear. (It looks like fill() and fillRect() also have this problem). Beyond that, a further optimization would be to clear only the region outside the destination rect. > > You can either of these in a future patch, but I would like to see at least the first optimization done at some point. Great point. The same optimization exists for the 'full canvas compositing' modes as well, in which case we can avoid creating a temporary buffer. Another optimization is to not create the temporary buffer for image drawing if the destination rect translates to an integral pixel rect in the device buffer. I was planning on adding these optimizations as a later patch, probably after all the drawImage variants are correct. > I would really prefer that we not introduce new canvas tests with pixel results, if possible. Pixels should be tested programmatically in JavaScript, using the fast/canvas/script-tests infrastructure. This avoids having to maintain platform-specific image baselines. In fact, doesn't the canvas/philip suite cover these modes? Or do they not have enough coverage? OK, that makes lots of sense. Unfortunately I didn't see the reference to the script-tests infrastructure until I had redone the test, instead I used the canvas-composite-alpha test as a template. Is it OK as it is? This test checks the composite mode as well as transformations and clip, so there are a lot of test points and a lot of static data. I added some helper functions to aid if there is ever the need to update the test (or generate a new similar one). The philip tests did not highlight this problem as the image being drawn is the same size as the canvas, so the case where there are parts of the canvas outside the image which get affected isn't covered. Also the philip tests for composited image drawing don't use transforms or clip. > > > LayoutTests/platform/chromium/test_expectations.txt:3588 > > +BUGWK66920 MAC WIN : fast/canvas/canvas-composite-image.html = IMAGE > > Doesn't this also affect Linux? It does but previous patch had expected pixel results for Linux.

Stephen White

Comment 14 2011-10-24 07:07:37 PDT

Comment on attachment 112138 [details] Patch This looks fine to me. Thanks very much for rewriting the test -- this will save the other ports some extra work. Please log a bug about the extra clear in the Copy case. Leaving for other reviewers (e.g., James, Tom) in case they have any comments.

James Robinson

Comment 15 2011-10-24 10:19:09 PDT

Comment on attachment 112138 [details] Patch Cool, R=me

WebKit Review Bot

Comment 16 2011-10-24 19:27:55 PDT

Comment on attachment 112138 [details] Patch Clearing flags on attachment: 112138 Committed r98309: <http://trac.webkit.org/changeset/98309>

WebKit Review Bot

Comment 17 2011-10-24 19:28:01 PDT

All reviewed patches have been landed. Closing bug.

Ryosuke Niwa

Comment 18 2011-10-25 00:44:45 PDT

Reopen the bug since the patch was rolled out.

Ben Wells

Comment 19 2011-10-25 22:40:34 PDT

Created attachment 112450 [details] Test updated to pass on all platforms.

Ben Wells

Comment 20 2011-10-25 22:51:44 PDT

Comment on attachment 112450 [details] Test updated to pass on all platforms. Color space removed from png in test; darker mode not tested any more (its not in the canvas spec).

James Robinson

Comment 21 2011-10-25 23:03:18 PDT

Comment on attachment 112450 [details] Test updated to pass on all platforms. R=me. Not touching cq yet since EWS hasn't cycled, but feel free to ask any committer to flip that once cr-linux (at least) goes green.

WebKit Review Bot

Comment 22 2011-10-26 00:51:15 PDT

Comment on attachment 112450 [details] Test updated to pass on all platforms. Clearing flags on attachment: 112450 Committed r98440: <http://trac.webkit.org/changeset/98440>

WebKit Review Bot

Comment 23 2011-10-26 00:51:22 PDT

All reviewed patches have been landed. Closing bug.

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

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Canvas

Assignee

Ben Wells

Reported

2011-08-24 20:42 PDT

Modified

2011-10-26 04:13 PDT History

CC List

10 users Show

URL

Keywords

Depends on

70801 70893

Blocks

Dependencies

tree graph