Bug 48291

Created attachment 95568 [details] Patch to check for CompositeCopy to paint transparency for remaining canvas area The existing code was only looking for CompositeSourceIn/CompositeSourceOut while painting transparency for the remaining canvas area.We have also added to check for CompositeCopy

Comment on attachment 95568 [details] Patch to check for CompositeCopy to paint transparency for remaining canvas area View in context: https://bugs.webkit.org/attachment.cgi?id=95568&action=review > Source/WebCore/ChangeLog:9 > + transparency for remaining canvas area The spec does not mention that we need to display transparency everywhere for copy. Could you explain why this is needed? Should the spec be changed or the test itself? > Source/WebCore/ChangeLog:11 > + Test: LayoutTests\canvas\philip\tests\2d.composite.uncovered.fill.copy.html This test is skipped on most platform. You need to re-enable it by removing it from the Skipped & test_expectations.txt files for all platforms.

(In reply to comment #2) > (From update of attachment 95568 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95568&action=review > > > Source/WebCore/ChangeLog:9 > > + transparency for remaining canvas area > > The spec does not mention that we need to display transparency everywhere for copy. Could you explain why this is needed? Should the spec be changed or the test itself? Yes, even I did not find anything explicitly mentioned in the Spec. I went by what is mentioned in test case. The spec mentions how Copy/fillRect should behave, but it does not mention what is the expected behavior in failure cases i.e. destination rect area out side the canvas area. Can anyone having more depth knowledge of the spec clarify this please?

> The spec mentions how Copy/fillRect should behave, but it does not mention what is the expected behavior in failure cases i.e. destination rect area out side the canvas area. > > Can anyone having more depth knowledge of the spec clarify this please? It looks like the spec is silent on this point. However Philip's test suite seems to be testing exactly that. Ian, what is your reading on this? Should we report that on the WHATWG or somewhere else?

What's the question, exactly?

(In reply to comment #5) > What's the question, exactly? Hi Ian, The HTML Canvas spec (http://www.w3.org/TR/2dcontext/#compositing) does not explicitly mentions what would be the exact behavior in case of an invalid copy operation. E.g. if i am copying an image to an area which is out side the valid canvas area, what would happen to my canvas area? Should it contain whatever was already painted or the canvas area would be transparent or something else?

The drawing model is described here: http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#drawing-model For _all_ the composition operators ('copy' isn't special here) the drawing model composites an infinite image over the canvas. The whole canvas is always affected.

As per discussion with Ian in IRC, this is a valid test case. So would any one be able to review this patch?

(In reply to comment #8) > As per discussion with Ian in IRC, this is a valid test case. So would any one be able to review this patch? I mentioned previously that this patch is missing an update of the platforms' Skipped files as you are fixing a test-case. The fix makes sense once taken into account the drawing model (a comment about that would be nice).

Created attachment 96720 [details] Patch after incorporating julien's review comments. Hi Julien, I have removed the relevant test case from skipped list [I forgot to do this before as I was completely involved in the discussion regarding the expected spec behavior with you & Ian :-(]. Also I have put a comment for my changes regarding the reference to drawing model as per your suggestion.

Comment on attachment 96720 [details] Patch after incorporating julien's review comments. cancelling the review by myself, as EWS reported problem applying the patch. Will investigate & upload the correct patch.

Created attachment 96749 [details] Updated Patch Updated patch after fixing the coding style issue & incorporating Julien's comment

Comment on attachment 96749 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96749&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1512 > + // As per the drawing model mentioned in http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#drawing-model > + // We should display transparency for everywhere else for CompositeCopy, too Where does it say that? Are you sure that's right? Section 4.8.11.1.3 mentions this for source-atop, source-in, and source-out. But not for copy.

(In reply to comment #13) > (From update of attachment 96749 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96749&action=review > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1512 > > + // As per the drawing model mentioned in http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#drawing-model > > + // We should display transparency for everywhere else for CompositeCopy, too > > Where does it say that? Are you sure that's right? > > Section 4.8.11.1.3 mentions this for source-atop, source-in, and source-out. But not for copy. Darin, Please refer to Ian's comment above [https://bugs.webkit.org/show_bug.cgi?id=48291#c7 ]. Also I had discussed with Ian in IRC to confirm this.

(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 96749 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=96749&action=review > > > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1512 > > > + // As per the drawing model mentioned in http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#drawing-model > > > + // We should display transparency for everywhere else for CompositeCopy, too > > > > Where does it say that? Are you sure that's right? > > > > Section 4.8.11.1.3 mentions this for source-atop, source-in, and source-out. But not for copy. > > Darin, Please refer to Ian's comment above [https://bugs.webkit.org/show_bug.cgi?id=48291#c7 ]. Also I had discussed with Ian in IRC to confirm this. Hi Darin, Do you want me to explain this further with more details or is it Ok to review this change now?

(In reply to comment #15) > Hi Darin, Do you want me to explain this further with more details or is it Ok to review this change now? You’ve answered my question. Someone can review your patch at any time. Since your patch did not apply in the EWS you probably want to make a new version that successfully applies. When I do get around to reviewing I will probably want to think a little further about whether there are any other modes that are handled wrong. Not reviewing right now. Someone else is welcome to review; please don’t wait for me just because I made a comment.

Created attachment 97068 [details] Recreated patch on the latest code base As the previous patch was not applied by the EWS, re-created the patch again.

Attachment 97068 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 LayoutTests/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] LayoutTests/ChangeLog:7: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:6: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] Source/WebCore/ChangeLog:11: Line contains tab character. [whitespace/tab] [5] Total errors found: 6 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 97070 [details] Fixed the "tabs" in Changelog reported by EWS in previous patch Fixed the style ("tab") issue reported by EWS

Comment on attachment 97070 [details] Fixed the "tabs" in Changelog reported by EWS in previous patch Attachment 97070 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8840321 New failing tests: fast/canvas/canvas-composite-alpha.html

Created attachment 97077 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 97097 [details] Patch after rebaselining the LayoutTests/fast/canvas/canvas-composite-alpha.html Updated patch after rebaselining the failed test case

Comment on attachment 97097 [details] Patch after rebaselining the LayoutTests/fast/canvas/canvas-composite-alpha.html View in context: https://bugs.webkit.org/attachment.cgi?id=97097&action=review > Source/WebCore/ChangeLog:11 > + Test: LayoutTests\canvas\philip\tests\2d.composite.uncovered.fill.copy.html Looks like you forgot to regenerate your changelog as it should mention canvas-composite-alpha.html here. > LayoutTests/ChangeLog:8 > + from failed/skipped test list For information specific to a file, we usually prefer to put them next or along to the file name(s) below in the ChangeLog. I usually put a global description here if it makes sense (like for big patches to explain the gist of it). See the other ChangeLog entries to see how other people do.

Comment on attachment 97097 [details] Patch after rebaselining the LayoutTests/fast/canvas/canvas-composite-alpha.html View in context: https://bugs.webkit.org/attachment.cgi?id=97097&action=review >> Source/WebCore/ChangeLog:11 >> + Test: LayoutTests\canvas\philip\tests\2d.composite.uncovered.fill.copy.html > > Looks like you forgot to regenerate your changelog as it should mention canvas-composite-alpha.html here. We don’t use backslashes for paths. We use forward slashes.

Created attachment 97332 [details] Incorporated review comments of Julien(comment#23) & Darin(comment#24) Updated patch after incorporating Julien & Darin's review comment

Comment on attachment 97332 [details] Incorporated review comments of Julien(comment#23) & Darin(comment#24) this looks sensible in the context of what's currently going on in the canvas, but has anyone actually been verifying that we haven't been getting any performance regressions from these changes? This especially important for "copy" as it's a reasonably common compositing mode (vs. atop, etc). Have we also verified that this behaviour matches other browsers?

(In reply to comment #26) > (From update of attachment 97332 [details]) > this looks sensible in the context of what's currently going on in the canvas, but has anyone actually been verifying that we haven't been getting any performance regressions from these changes? This especially important for "copy" as it's a reasonably common compositing mode (vs. atop, etc). Have we also verified that this behaviour matches other browsers? I haven't verified the performance related aspect because to be compliant with the spec behavior, we need this change (as per my understanding). Secondly, I have verified that this behavior after my changes match with FF4 & Opera 11.

(In reply to comment #26) > (From update of attachment 97332 [details]) > this looks sensible in the context of what's currently going on in the canvas, but has anyone actually been verifying that we haven't been getting any performance regressions from these changes? This especially important for "copy" as it's a reasonably common compositing mode (vs. atop, etc). Have we also verified that this behaviour matches other browsers? Regarding your comment ("I suspect that at the moment we're taking a huge hit if you do a copy into the canvas that fills the entire canvas") during our discussion on IRC, I did some further debugging & found that if my copy is filling entire canvas, i am not clearing out unnecessarily. So, as per my observation, we are only clearing that area which we are not going to fill by our copy operation. Can mdelaney/kling please comment here?

(In reply to comment #26) > (From update of attachment 97332 [details]) > this looks sensible in the context of what's currently going on in the canvas, but has anyone actually been verifying that we haven't been getting any performance regressions from these changes? This especially important for "copy" as it's a reasonably common compositing mode (vs. atop, etc). Have we also verified that this behaviour matches other browsers? Hi Oliver, I have tried a CompositeCopy on a 1000*500 canvas [copying to the entire canvas region] on WebKit windows build [Winlauncher]. In each iterations, I am doing fillRect for 100 times & this whole exercise I have done for 10 iterations to get a stable result[loading the page 10 times and noting the result]. With Changes: Total time taken(sum of 10 iterations) = 1139 ms ==> avg=1.13 ms per fillRect WithOUT Changes: Total time taken(sum of 10 iterations) = 1020 ms ==> avg=1.02 ms per fillRect. Please let me know your opinion & whether you want any modification in the patch/in the existing logic of this filling transparency elsewhere.

Created attachment 98157 [details] Test case to take performance figure for CompositeCopy Attaching the test case I have used to take the performance figure for CompositeCopy

(In reply to comment #30) > Created an attachment (id=98157) [details] > Test case to take performance figure for CompositeCopy > > Attaching the test case I have used to take the performance figure for CompositeCopy The above test case is modified version of 2d.composite.uncovered.fill.copy.html which should be run from LayoutTests/canvas/philip/tests/

Comment on attachment 97332 [details] Incorporated review comments of Julien(comment#23) & Darin(comment#24) View in context: https://bugs.webkit.org/attachment.cgi?id=97332&action=review I'd be interesting in seeing perf comparisons that do not cover the entire canvas. If the entire canvas is being copied then the clipOut() will exclude the whole canvas so that fill will be a no-op, so it's not surprising that performance is unaffected. I'm also not sure about using clipOut for copy - it seems that for a path that isn't exactly on pixel boundaries this will cause issues with antialiasing. If the clip region is small relative to the canvas, would it be more efficient to just clear the whole canvas without setting a clipOut at all before the actual draw? On GPU implementations I could imagine this doing quite well. This will change the pixel output for fast/canvas/canvas-composite.html, at least. Could you update the pixel expectations for at least one platform so we can see that it's correct? > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1514 > + // As per the drawing model mentioned in http://www.whatwg.org/specs/web-apps/current-work/complete/the-canvas-element.html#drawing-model > + // We should display transparency for everywhere else for CompositeCopy, too This comment does not seem useful, the comment on 1510 explains why we are displaying transparency elsewhere

*** This bug has been marked as a duplicate of bug 66036 ***