Bug 48291

Summary: Fix LayoutTests/canvas/philip/tests/2d.composite.uncovered.fill.copy.html
Product: WebKit Reporter: Mike Lawther <mikelawther>
Component: CanvasAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: ariya.hidayat, benwells, darin, dglazkov, eric, excors, ian, jamesr, jchaffraix, kling, mdelaney7, mustaf.here, vswap65, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 46506    
Attachments:
Description Flags
Patch to check for CompositeCopy to paint transparency for remaining canvas area
none
Patch after incorporating julien's review comments.
none
Updated Patch
none
Recreated patch on the latest code base
none
Fixed the "tabs" in Changelog reported by EWS in previous patch
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
Patch after rebaselining the LayoutTests/fast/canvas/canvas-composite-alpha.html
none
Incorporated review comments of Julien(comment#23) & Darin(comment#24)
jamesr: review-
Test case to take performance figure for CompositeCopy none

Description Mike Lawther 2010-10-25 22:20:22 PDT
This layout tests fails. See master bug: https://bugs.webkit.org/show_bug.cgi?id=46506
Comment 1 Swapna P 2011-06-01 02:25:36 PDT
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 2 Julien Chaffraix 2011-06-02 07:34:25 PDT
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.
Comment 3 Mustafizur Rahaman (rahaman) 2011-06-02 10:35:57 PDT
(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?
Comment 4 Julien Chaffraix 2011-06-06 20:34:53 PDT
> 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?
Comment 5 Ian 'Hixie' Hickson 2011-06-06 22:42:20 PDT
What's the question, exactly?
Comment 6 Mustafizur Rahaman (rahaman) 2011-06-06 23:07:05 PDT
(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?
Comment 7 Ian 'Hixie' Hickson 2011-06-06 23:20:57 PDT
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.
Comment 8 Mustafizur Rahaman (rahaman) 2011-06-07 11:06:03 PDT
As per discussion with Ian in IRC, this is a valid test case. So would any one be able to review this patch?
Comment 9 Julien Chaffraix 2011-06-09 17:33:00 PDT
(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).
Comment 10 Swapna P 2011-06-10 03:02:58 PDT
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 11 Swapna P 2011-06-10 05:35:18 PDT
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.
Comment 12 Mustafizur Rahaman (rahaman) 2011-06-10 09:35:44 PDT
Created attachment 96749 [details]
Updated Patch

Updated patch after fixing the coding style issue & incorporating Julien's comment
Comment 13 Darin Adler 2011-06-10 09:54:44 PDT
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.
Comment 14 Mustafizur Rahaman (rahaman) 2011-06-10 10:01:57 PDT
(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.
Comment 15 Mustafizur Rahaman (rahaman) 2011-06-12 01:22:42 PDT
(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?
Comment 16 Darin Adler 2011-06-12 18:25:42 PDT
(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.
Comment 17 Mustafizur Rahaman (rahaman) 2011-06-13 22:43:12 PDT
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.
Comment 18 WebKit Review Bot 2011-06-13 22:45:32 PDT
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.
Comment 19 Mustafizur Rahaman (rahaman) 2011-06-13 22:51:10 PDT
Created attachment 97070 [details]
Fixed the "tabs" in Changelog reported by EWS in previous patch

Fixed the style ("tab") issue reported by EWS
Comment 20 WebKit Review Bot 2011-06-14 00:07:23 PDT
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
Comment 21 WebKit Review Bot 2011-06-14 00:07:28 PDT
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
Comment 22 Mustafizur Rahaman (rahaman) 2011-06-14 05:07:49 PDT
Created attachment 97097 [details]
Patch after rebaselining the LayoutTests/fast/canvas/canvas-composite-alpha.html

Updated patch after rebaselining the failed test case
Comment 23 Julien Chaffraix 2011-06-14 07:22:00 PDT
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 24 Darin Adler 2011-06-14 09:55:50 PDT
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.
Comment 25 Mustafizur Rahaman (rahaman) 2011-06-15 11:19:40 PDT
Created attachment 97332 [details]
Incorporated review comments of Julien(comment#23) & Darin(comment#24)

Updated patch after incorporating Julien & Darin's review comment
Comment 26 Oliver Hunt 2011-06-15 12:25:57 PDT
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?
Comment 27 Mustafizur Rahaman (rahaman) 2011-06-15 12:30:43 PDT
(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.
Comment 28 Mustafizur Rahaman (rahaman) 2011-06-15 13:49:00 PDT
(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?
Comment 29 Mustafizur Rahaman (rahaman) 2011-06-22 04:50:48 PDT
(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.
Comment 30 Mustafizur Rahaman (rahaman) 2011-06-22 04:53:13 PDT
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
Comment 31 Mustafizur Rahaman (rahaman) 2011-06-22 05:04:04 PDT
(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 32 James Robinson 2011-06-24 14:09:23 PDT
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
Comment 33 Ben Wells 2011-08-22 23:18:26 PDT

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