Created attachment 105127 [details] Test case These have the same errors as were recently fixed in bug 66036.
Working on this.
Created attachment 110970 [details] Patch for comment, patch with tests to come.
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.
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?
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.
Created attachment 111198 [details] Patch which doesn't merge.
Created attachment 111202 [details] Patch with function made static non-member, and test added.
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?
Created attachment 111529 [details] Patch
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.
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?
Created attachment 112138 [details] Patch
(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.
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.
Comment on attachment 112138 [details] Patch Cool, R=me
Comment on attachment 112138 [details] Patch Clearing flags on attachment: 112138 Committed r98309: <http://trac.webkit.org/changeset/98309>
All reviewed patches have been landed. Closing bug.
Reopen the bug since the patch was rolled out.
Created attachment 112450 [details] Test updated to pass on all platforms.
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).
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.
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>